[GitHub] carbondata pull request #2826: [CARBONDATA-3023] Alter add column issue with...

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2826#discussion_r228056940
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/filter/executer/RangeValueFilterExecuterImpl.java ---
    @@ -511,7 +511,7 @@ private BitSet setFilterdIndexToBitSetWithColumnIndex(
     
         // Binary Search cannot be done on '@NU#LL$!", so need to check and compare for null on
         // matching row.
    -    if (dimensionColumnPage.isNoDicitionaryColumn()) {
    +    if (dimensionColumnPage.isNoDicitionaryColumn() && !dimensionColumnPage.isAdaptiveEncoded()) {
    --- End diff --
   
    Not related to this PR. But I think we better have a function to return the encoding type of the columnPage instead of having `isAdaptiveEncoded`, since we will add more encoding in the future.
    @ravipesala @ajantha-bhat  please check


---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2826#discussion_r228498210
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/filter/executer/RangeValueFilterExecuterImpl.java ---
    @@ -511,7 +511,7 @@ private BitSet setFilterdIndexToBitSetWithColumnIndex(
     
         // Binary Search cannot be done on '@NU#LL$!", so need to check and compare for null on
         // matching row.
    -    if (dimensionColumnPage.isNoDicitionaryColumn()) {
    +    if (dimensionColumnPage.isNoDicitionaryColumn() && !dimensionColumnPage.isAdaptiveEncoded()) {
    --- End diff --
   
    @jackylk isAdaptiveEncoded() is already set/decided by the function org.apache.carbondata.core.datastore.chunk.reader.dimension.v3.CompressedDimensionChunkFileBasedReaderV3#isEncodedWithAdaptiveMeta. In future also we can change the function accordingly


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2826#discussion_r228500224
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/filter/executer/RangeValueFilterExecuterImpl.java ---
    @@ -511,7 +511,7 @@ private BitSet setFilterdIndexToBitSetWithColumnIndex(
     
         // Binary Search cannot be done on '@NU#LL$!", so need to check and compare for null on
         // matching row.
    -    if (dimensionColumnPage.isNoDicitionaryColumn()) {
    +    if (dimensionColumnPage.isNoDicitionaryColumn() && !dimensionColumnPage.isAdaptiveEncoded()) {
    --- End diff --
   
    @jackylk : yes, we can have a BitSet in columnPage for enum of Encoding, and from PageMetaData encodings we can set the bitset. And to check whether particular encoding is there or not, we just have to check whether that bit is set or not.
   
    Currently we don't need this, but like you said, it will help for future changes. I can do this change if you and @ravipesala  agrees for this change.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2826#discussion_r228542847
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/util/CarbonScalaUtil.scala ---
    @@ -671,4 +671,20 @@ object CarbonScalaUtil {
         dataType == StringType
       }
     
    +  /**
    +   * Rearrange the column schema with all the sort columns at first. In case of ALTER ADD COLUMNS,
    +   * if the newly added column is a sort column it will be at the last. But we expects all the
    +   * SORT_COLUMNS always at first
    +   *
    +   * @param columnSchemas
    +   * @return
    +   */
    +  def reArrangeColumnSchema(columnSchemas: mutable.Buffer[ColumnSchema]): mutable
    +  .Buffer[ColumnSchema] = {
    --- End diff --
   
    alterTableModel.tableProperties.get("sort_columns") will give size sort columns,
   
    so based on this we can traverse only once and set both sort and non-sort columns in buffer, just set sort columns in beginning and non-sort column index start from size of sortcolumn.



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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


---