[GitHub] carbondata pull request #2326: [WIP] sortColumn with empty gives exception

[qiuchenjian-2]

GitHub user rahulforallp opened a pull request:

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

    [WIP] sortColumn with empty gives exception

    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     
     - [ ] Any backward compatibility impacted?
     
     - [ ] Document update required?
   
     - [ ] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
            - How it is tested? Please attach test report.
            - Is it a performance related change? Please attach the performance test report.
            - Any additional information to help reviewers in testing this change.
           
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
   


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/rahulforallp/incubator-carbondata sort_col_sdk_emptyVal

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/2326.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2326
   
----
commit 4ac783289913c267e04bf2cfc558791e4caabd6b
Author: rahulforallp <rahul.kumar@...>
Date:   2018-05-21T09:47:10Z

    sortColumn with empty gives exception is fixed

commit 4c2e32ec0df62aec94384d21ee1b6edccf072a54
Author: dhatchayani <dhatcha.official@...>
Date:   2018-05-21T09:19:37Z

    Update issue on select query

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2326
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4850/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2326#discussion_r189787603
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java ---
    @@ -406,8 +406,9 @@ private CarbonTable buildCarbonTable() {
         ColumnSchema[] sortColumnsSchemaList = new ColumnSchema[sortColumnsList.size()];
         Field[] fields = schema.getFields();
         buildTableSchema(fields, tableSchemaBuilder, sortColumnsList, sortColumnsSchemaList);
    -
    -    tableSchemaBuilder.setSortColumns(Arrays.asList(sortColumnsSchemaList));
    +    if (!(sortColumnsSchemaList.length == 1 && sortColumnsSchemaList[0] == null)) {
    --- End diff --
   
    this scenario should not handled....in this case throw exception


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2326#discussion_r189787714
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java ---
    @@ -431,18 +432,20 @@ private void buildTableSchema(Field[] fields, TableSchemaBuilder tableSchemaBuil
         // to child of complex array type in the order val1, val2 so that each array type child is
         // differentiated to any level
         AtomicInteger valIndex = new AtomicInteger(0);
    -    // Check if any of the columns specified in sort columns are missing from schema.
    -    for (String sortColumn: sortColumnsList) {
    -      boolean exists = false;
    -      for (Field field : fields) {
    -        if (field.getFieldName().equalsIgnoreCase(sortColumn)) {
    -          exists = true;
    -          break;
    +    if (!(sortColumnsList.size() == 1 && sortColumnsList.get(0).trim().isEmpty())) {
    --- End diff --
   
    same comment as above...not required to handle this case


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2326#discussion_r189787853
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java ---
    @@ -488,6 +491,8 @@ private void buildTableSchema(Field[] fields, TableSchemaBuilder tableSchemaBuil
                 columnSchema.setSortColumn(true);
                 sortColumnsSchemaList[i] = columnSchema;
                 i++;
    +          } else {
    +            columnSchema.setSortColumn(false);
    --- End diff --
   
    its a boolean value..by default it will be false so this code change is not required


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2326#discussion_r189788038
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/createTable/TestNonTransactionalCarbonTable.scala ---
    @@ -379,6 +377,32 @@ class TestNonTransactionalCarbonTable extends QueryTest with BeforeAndAfterAll {
     
         checkExistence(sql("describe formatted sdkOutputTable"), true, "name")
     
    +    buildTestDataWithSortColumns(List())
    +    assert(new File(writerPath).exists())
    +    sql("DROP TABLE IF EXISTS sdkOutputTable")
    +
    +    // with partition
    +    sql(
    +      s"""CREATE EXTERNAL TABLE sdkOutputTable(name string) PARTITIONED BY (age int) STORED BY
    +         |'carbondata' LOCATION
    +         |'$writerPath' """.stripMargin)
    +
    +    sql("describe formatted sdkOutputTable").show(false)
    +    sql("select * from sdkOutputTable").show()
    +
    +    buildTestDataWithSortColumns(List(""))
    --- End diff --
   
    in this test case the failure exception should come..so modify the test case to intercept for exception here


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2326
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4882/



---

[qiuchenjian-2]

Github user manishgupta88 commented on the issue:

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


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---