[GitHub] carbondata pull request #2902: [WIP] Fixed data mismatch issue after update

[qiuchenjian-2]

GitHub user kunal642 opened a pull request:

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

    [WIP] Fixed data mismatch issue after update

    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/kunal642/carbondata update_data_mismatch_fix

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

    https://github.com/apache/carbondata/pull/2902.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 #2902
   
----
commit bbd3dc87ac84c1d4005379dd445dec30f31f24aa
Author: kunal642 <kunalkapoor642@...>
Date:   2018-11-06T05:21:00Z

    fixed data mismatch issue after update

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2902#discussion_r231009938
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1734,7 +1734,7 @@ private CarbonCommonConstants() {
       public static final String CARBON_PUSH_ROW_FILTERS_FOR_VECTOR =
           "carbon.push.rowfilters.for.vector";
     
    -  public static final String CARBON_PUSH_ROW_FILTERS_FOR_VECTOR_DEFAULT = "false";
    +  public static final String CARBON_PUSH_ROW_FILTERS_FOR_VECTOR_DEFAULT = "true";
    --- End diff --
   
    Any specific reason for changing the default value?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2902
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1512/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2902
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9559/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2902#discussion_r231014429
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/iud/UpdateCarbonTableTestCase.scala ---
    @@ -772,12 +772,33 @@ class UpdateCarbonTableTestCase extends QueryTest with BeforeAndAfterAll {
         sql("""drop table if exists iud.dest33_part""")
       }
     
    +  test("check data after update with row.filter pushdown as false") {
    +    CarbonProperties.getInstance().addProperty(CarbonCommonConstants
    +      .CARBON_PUSH_ROW_FILTERS_FOR_VECTOR, "false")
    +    sql("""drop table if exists iud.dest33_flat""")
    +    sql(
    +      """create table iud.dest33_part (c1 int,c2 string, c3 short) STORED BY 'carbondata'"""
    +        .stripMargin)
    +    sql(
    +      s"""LOAD DATA LOCAL INPATH '$resourcesPath/IUD/negativevalue.csv' INTO table iud
    +         |.dest33_part options('header'='false')""".stripMargin)
    +    sql(
    +      """update iud.dest33_part d set (c1) = (5) where d.c1 = 0""".stripMargin).show()
    +    checkAnswer(sql("select c3 from iud.dest33_part"), Seq(Row(-300), Row(0), Row(-200), Row(700)
    +      , Row(100), Row(-100), Row(null)))
    +    sql("""drop table if exists iud.dest33_part""")
    +    CarbonProperties.getInstance().addProperty(CarbonCommonConstants
    +      .CARBON_PUSH_ROW_FILTERS_FOR_VECTOR, "true")
    --- End diff --
   
    After test case completion we should set the default value for `CARBON_PUSH_ROW_FILTERS_FOR_VECTOR`?...default property is false so I think at the start of test case no need to modify the property value


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2902#discussion_r231016130
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/compress/DirectCompressCodec.java ---
    @@ -257,7 +265,13 @@ private void fillVector(ColumnPage columnPage, CarbonColumnVector vector,
           } else if (pageDataType == DataTypes.SHORT) {
             short[] shortData = columnPage.getShortPage();
             if (vectorDataType == DataTypes.SHORT) {
    -          vector.putShorts(0, pageSize, shortData, 0);
    +          if (isUnderlyingVectorPresent) {
    +            for (int i = 0; i < pageSize; i++) {
    +              vector.putShort(i, shortData[i]);
    +            }
    +          } else {
    +            vector.putShorts(0, pageSize, shortData, 0);
    --- End diff --
   
    I think using `putShorts/putFloats` is common and unavoidable. In future also any new encoding class can make use of these method and then again the same problem can occur. Is it feasible to modify the vector classes implementation methods itself just like an example below
    `public void putShorts(int rowId, int count, short[] src, int srcIndex) {
        for (int i = srcIndex; i < count; i++) {
          putShort(rowId++, src[i]);
        }
      }`
    This way it will be better


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2902#discussion_r231012403
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/iud/UpdateCarbonTableTestCase.scala ---
    @@ -772,12 +772,33 @@ class UpdateCarbonTableTestCase extends QueryTest with BeforeAndAfterAll {
         sql("""drop table if exists iud.dest33_part""")
       }
     
    +  test("check data after update with row.filter pushdown as false") {
    +    CarbonProperties.getInstance().addProperty(CarbonCommonConstants
    +      .CARBON_PUSH_ROW_FILTERS_FOR_VECTOR, "false")
    +    sql("""drop table if exists iud.dest33_flat""")
    +    sql(
    +      """create table iud.dest33_part (c1 int,c2 string, c3 short) STORED BY 'carbondata'"""
    +        .stripMargin)
    +    sql(
    +      s"""LOAD DATA LOCAL INPATH '$resourcesPath/IUD/negativevalue.csv' INTO table iud
    +         |.dest33_part options('header'='false')""".stripMargin)
    +    sql(
    +      """update iud.dest33_part d set (c1) = (5) where d.c1 = 0""".stripMargin).show()
    +    checkAnswer(sql("select c3 from iud.dest33_part"), Seq(Row(-300), Row(0), Row(-200), Row(700)
    +      , Row(100), Row(-100), Row(null)))
    +    sql("""drop table if exists iud.dest33_part""")
    +    CarbonProperties.getInstance().addProperty(CarbonCommonConstants
    +      .CARBON_PUSH_ROW_FILTERS_FOR_VECTOR, "true")
    +  }
    +
       override def afterAll {
         sql("use default")
         sql("drop database  if exists iud cascade")
         CarbonProperties.getInstance()
           .addProperty(CarbonCommonConstants.isHorizontalCompactionEnabled , "true")
         CarbonProperties.getInstance()
           .addProperty(CarbonCommonConstants.ENABLE_VECTOR_READER , "true")
    +    CarbonProperties.getInstance().addProperty(CarbonCommonConstants
    +      .CARBON_PUSH_ROW_FILTERS_FOR_VECTOR, "false")
    --- End diff --
   
    instead of hard coding `"false"` use default value from constants


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2902
 
    @kunal642 Please check PR https://github.com/apache/carbondata/pull/2863 . This issue should not happen there. Please verify once


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2902
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9564/



---

[qiuchenjian-2]

Github user kunal642 closed the pull request at:

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


---