[GitHub] carbondata pull request #2983: [CARBONDATA-3119] Fixed SDK Write for Complex...

[qiuchenjian-2]

GitHub user shivamasn opened a pull request:

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

    [CARBONDATA-3119] Fixed SDK Write for Complex Array Type when Array is Empty

    ### What was the issue?
    For SDK Write , it was going into bad record by returning null on passing empty array for Complex Type.
    ### What has been changed?
    Added a check for empty array. This will return an empty array.
   
     - [ ] Any interfaces changed?
     NA
     - [ ] Any backward compatibility impacted?
     NA
     - [ ] Document update required?
    NA
     - [ ] 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.
    Test  case added
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
    NA


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

    $ git pull https://github.com/shivamasn/carbondata complex_issue

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

    https://github.com/apache/carbondata/pull/2983.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 #2983
   
----
commit a81911b32aa7b586a5ae0c8d206d16fb8921c6fa
Author: namanrastogi <naman.rastogi.52@...>
Date:   2018-12-10T13:34:43Z

    Complex Type Empty String Issue Fixed

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2983#discussion_r240889074
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/complexType/TestComplexDataType.scala ---
    @@ -39,6 +39,27 @@ class TestComplexDataType extends QueryTest with BeforeAndAfterAll {
           .addProperty(CarbonCommonConstants.CARBON_BAD_RECORDS_ACTION, badRecordAction)
       }
     
    +  test("test Projection PushDown for Array - String type when Array is Empty") {
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonCommonConstants.CARBON_BAD_RECORDS_ACTION, "FAIL")
    --- End diff --
   
    Take a previous CARBON_BAD_RECORDS_ACTION property as backup and set it back after this test case, else it affects other test suites


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2983#discussion_r240889108
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/complexType/TestComplexDataType.scala ---
    @@ -39,6 +39,27 @@ class TestComplexDataType extends QueryTest with BeforeAndAfterAll {
           .addProperty(CarbonCommonConstants.CARBON_BAD_RECORDS_ACTION, badRecordAction)
       }
     
    +  test("test Projection PushDown for Array - String type when Array is Empty") {
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonCommonConstants.CARBON_BAD_RECORDS_ACTION, "FAIL")
    +    sql("drop table if exists table1")
    +    sql("create table table1 (detail array<string>) stored by 'carbondata'")
    +    sql("insert into table1 values('')")
    +    checkAnswer(sql("select detail[0] from table1"), Seq(Row("")))
    +    sql("drop table if exists table1")
    +  }
    +
    +  test("test Projection PushDown for Struct - Array type when Array is Empty") {
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonCommonConstants.CARBON_BAD_RECORDS_ACTION, "FAIL")
    --- End diff --
   
    same as above


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2983#discussion_r240889185
 
    --- Diff: store/sdk/src/test/java/org/apache/carbondata/sdk/file/CarbonReaderTest.java ---
    @@ -23,8 +23,11 @@
     import java.util.*;
     
     import org.apache.avro.generic.GenericData;
    +
    --- End diff --
   
    please revert unwanted changes, only your changes should be there in diff


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2983#discussion_r240889253
 
    --- Diff: store/sdk/src/test/java/org/apache/carbondata/sdk/file/CarbonReaderTest.java ---
    @@ -2072,4 +1797,35 @@ public void testReadingNullValues() {
         }
       }
     
    +  @Test public void testSdkWriteWhenArrayOfStringIsEmpty()
    +      throws IOException, InvalidLoadOptionException {
    +
    +    CarbonProperties.getInstance()
    +        .addProperty(CarbonCommonConstants.CARBON_BAD_RECORDS_ACTION, "FAIL");
    --- End diff --
   
    same as above, take a backup of CARBON_BAD_RECORDS_ACTION and set it at the end of the test case


---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    @ravipesala , @chenliang613 : please add @shivamasn to whitelist


---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

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


---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

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


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    add to whitelist


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1720/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1932/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    Build Failed  with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9980/



---

[qiuchenjian-2]

Github user shivamasn commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1721/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1933/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    Build Failed  with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9981/



---

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    add to whitelist


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1730/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2983
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1943/



---