[GitHub] carbondata pull request #1603: [CARBONDATA-1844] Add tablePath support when ...

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1603#discussion_r154527728
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/TableInfo.java ---
    @@ -34,6 +34,8 @@
     import org.apache.carbondata.core.metadata.schema.table.column.ColumnSchema;
     import org.apache.carbondata.core.metadata.schema.table.column.ParentColumnTableRelation;
     
    +import org.apache.http.annotation.Obsolete;
    --- End diff --
   
    Better use java.lang.Deprecated


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1603
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/1656/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1603
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2046/



---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1603#discussion_r154528596
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -854,6 +854,7 @@
       public static final String DICTIONARY_EXCLUDE = "dictionary_exclude";
       public static final String DICTIONARY_INCLUDE = "dictionary_include";
       public static final String SORT_COLUMNS = "sort_columns";
    +  public static final String TABLE_PATH = "table_path";
    --- End diff --
   
    I think this will be confusion if we use 2 different words `tablePath` and `table_path`, better use only one `tablePath`.


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1603#discussion_r154528674
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/dataload/TestLoadDataWithHiveSyntaxDefaultFormat.scala ---
    @@ -687,6 +688,48 @@ class TestLoadDataWithHiveSyntaxDefaultFormat extends QueryTest with BeforeAndAf
         checkAnswer(sql("select salary from double_test limit 1"),Row(7.756787654567891E23))
       }
     
    +  test("test table with specified table path") {
    +    val path = "./source"
    +    sql("drop table if exists table_path_test")
    +    sql(
    +      "CREATE table table_path_test (empno string, salary double) STORED BY 'carbondata' " +
    +      s"TBLPROPERTIES('table_path'='$path')"
    --- End diff --
   
    We should not take path from tableproperties . Better use standard way of specifying the path. For both spark datasource and hive ddl uses [LOCATION path] to take tablePath. Better use the same.


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1603#discussion_r154530400
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/dataload/TestLoadDataWithHiveSyntaxDefaultFormat.scala ---
    @@ -687,6 +688,48 @@ class TestLoadDataWithHiveSyntaxDefaultFormat extends QueryTest with BeforeAndAf
         checkAnswer(sql("select salary from double_test limit 1"),Row(7.756787654567891E23))
       }
     
    +  test("test table with specified table path") {
    +    val path = "./source"
    +    sql("drop table if exists table_path_test")
    +    sql(
    +      "CREATE table table_path_test (empno string, salary double) STORED BY 'carbondata' " +
    +      s"TBLPROPERTIES('table_path'='$path')"
    --- End diff --
   
    ok


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1603#discussion_r154551395
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/TableInfo.java ---
    @@ -68,17 +70,10 @@
        */
       private long lastUpdatedTime;
     
    -  /**
    -   * metadata file path (check if it is really required )
    -   */
    -  private String metaDataFilepath;
    -
    -  /**
    -   * store location
    -   */
    +  @Obsolete
    --- End diff --
   
    fixed, it is not obsoleted


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1603#discussion_r154551403
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/TableInfo.java ---
    @@ -34,6 +34,8 @@
     import org.apache.carbondata.core.metadata.schema.table.column.ColumnSchema;
     import org.apache.carbondata.core.metadata.schema.table.column.ParentColumnTableRelation;
     
    +import org.apache.http.annotation.Obsolete;
    --- End diff --
   
    fixed


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1603#discussion_r154551444
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -854,6 +854,7 @@
       public static final String DICTIONARY_EXCLUDE = "dictionary_exclude";
       public static final String DICTIONARY_INCLUDE = "dictionary_include";
       public static final String SORT_COLUMNS = "sort_columns";
    +  public static final String TABLE_PATH = "table_path";
    --- End diff --
   
    ok, I will user LOCATION for hive syntax and tablePath for datasource table in dataframe option


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1603#discussion_r154552516
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/dataload/TestLoadDataWithHiveSyntaxDefaultFormat.scala ---
    @@ -687,6 +688,47 @@ class TestLoadDataWithHiveSyntaxDefaultFormat extends QueryTest with BeforeAndAf
         checkAnswer(sql("select salary from double_test limit 1"),Row(7.756787654567891E23))
       }
     
    +  test("test table with specified table path") {
    +    val path = "./source"
    +    sql("drop table if exists table_path_test")
    +    sql(
    +      "CREATE table table_path_test (empno string, salary double) STORED BY 'carbondata' " +
    +      s"LOCATION '$path'"
    --- End diff --
   
    changed to use LOCATION syntax


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1603#discussion_r154552813
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/dataload/TestLoadDataFrame.scala ---
    @@ -222,6 +223,26 @@ test("test the boolean data type"){
         )
       }
     
    +  test("test datasource table with specified table path") {
    --- End diff --
   
    This is new testcase for datasource table


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1603
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2048/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1603
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/1658/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1603
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2050/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1603
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/1660/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1603
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1603
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/1663/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1603
 
    LGTM


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

    https://github.com/apache/carbondata/pull/1603


---