[GitHub] carbondata pull request #2469: Added fix for Local Dictionary Exclude for mu...

[qiuchenjian-2]

GitHub user praveenmeenakshi56 opened a pull request:

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

    Added fix for Local Dictionary Exclude for multi level complex columns

    ### What was the problem?
    When Local Dictionary Exclude was defined for multi level complex columns, the columns were still considered for Local Dictionary Include
   
    ### What has been changed?
    The index value was not getting updated on return from the recursive method needed for traversal.
   
     - [ ] 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/praveenmeenakshi56/carbondata local_dict_complex_fix

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

    https://github.com/apache/carbondata/pull/2469.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 #2469
   
----
commit 7dab21ce5df001235d7956e0b4eb32bedf3bee22
Author: praveenmeenakshi56 <praveenmeenakshi56@...>
Date:   2018-07-09T15:06:36Z

    Added fix for Local Dictionary Exclude for multi level complex columns

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2469#discussion_r201263470
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/localdictionary/LocalDictionarySupportAlterTableTest.scala ---
    @@ -1161,6 +1159,126 @@ class LocalDictionarySupportAlterTableTest extends QueryTest with BeforeAndAfter
         }
       }
     
    +  test("test alter for local dictionary for complex columns when local dictionary exclude is defined _001") {
    +    sql("drop table if exists local1")
    +    sql(
    +      """
    +        | CREATE TABLE local1(id int, name string,city string, st array<struct<si:int,sd:string>>)
    +        | STORED BY 'org.apache.carbondata.format'
    +        | tblproperties('long_string_columns'='name','local_dictionary_enable'='true')
    +      """.stripMargin)
    +    sql("alter table local1 set tblproperties('local_dictionary_exclude'='st,name')")
    +    val descLoc = sql("describe formatted local1").collect
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Threshold")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("10000"))
    --- End diff --
   
    add one more None case, which asserts false, for all the test cases where you are checking describe formatted.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2469#discussion_r201264110
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/localdictionary/LocalDictionarySupportAlterTableTest.scala ---
    @@ -1161,6 +1159,126 @@ class LocalDictionarySupportAlterTableTest extends QueryTest with BeforeAndAfter
         }
       }
     
    +  test("test alter for local dictionary for complex columns when local dictionary exclude is defined _001") {
    +    sql("drop table if exists local1")
    +    sql(
    +      """
    +        | CREATE TABLE local1(id int, name string,city string, st array<struct<si:int,sd:string>>)
    +        | STORED BY 'org.apache.carbondata.format'
    +        | tblproperties('long_string_columns'='name','local_dictionary_enable'='true')
    +      """.stripMargin)
    +    sql("alter table local1 set tblproperties('local_dictionary_exclude'='st,name')")
    +    val descLoc = sql("describe formatted local1").collect
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Threshold")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("10000"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Enabled")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("true"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Exclude")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("st.val.sd,name"))
    --- End diff --
   
    Local Dictionary Excludem should contain child columns sd and si , both right?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2469#discussion_r201261685
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java ---
    @@ -3185,11 +3185,15 @@ private static int unsetLocalDictForComplexColumns(List<ColumnSchema> allColumns
           ColumnSchema column = allColumns.get(dimensionOrdinal);
           if (column.getNumberOfChild() > 0) {
             dimensionOrdinal++;
    -        unsetLocalDictForComplexColumns(allColumns, dimensionOrdinal, column.getNumberOfChild());
    +        // Value of dimensionOrdinal returned from recursive function was not considered
    --- End diff --
   
    do not give the reason in comment, why this issue was coming, add proper comment


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2469#discussion_r201264662
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/localdictionary/LocalDictionarySupportAlterTableTest.scala ---
    @@ -1161,6 +1159,126 @@ class LocalDictionarySupportAlterTableTest extends QueryTest with BeforeAndAfter
         }
       }
     
    +  test("test alter for local dictionary for complex columns when local dictionary exclude is defined _001") {
    +    sql("drop table if exists local1")
    +    sql(
    +      """
    +        | CREATE TABLE local1(id int, name string,city string, st array<struct<si:int,sd:string>>)
    +        | STORED BY 'org.apache.carbondata.format'
    +        | tblproperties('long_string_columns'='name','local_dictionary_enable'='true')
    +      """.stripMargin)
    +    sql("alter table local1 set tblproperties('local_dictionary_exclude'='st,name')")
    +    val descLoc = sql("describe formatted local1").collect
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Threshold")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("10000"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Enabled")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("true"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Exclude")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("st.val.sd,name"))
    +    }
    +  }
    +
    +  test("test alter for local dictionary for complex columns when local dictionary exclude is defined _002") {
    +    sql("drop table if exists local1")
    +    sql(
    +      """
    +        | CREATE TABLE local1(id int, name string,city string, st array<struct<si:int,sd:string>>,f string,g int,h string)
    +        | STORED BY 'org.apache.carbondata.format'
    +        | tblproperties('long_string_columns'='name','local_dictionary_enable'='true','local_dictionary_include'='st')
    +      """.stripMargin)
    +    sql("alter table local1 unset tblproperties('local_dictionary_include')")
    +    sql("alter table local1 set tblproperties('local_dictionary_exclude'='st,name,h')")
    +    val descLoc = sql("describe formatted local1").collect
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Threshold")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("10000"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Enabled")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("true"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Exclude")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("h,st.val.sd,name"))
    --- End diff --
   
    add all the child excluded columns in the check


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2469#discussion_r201267432
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/localdictionary/LocalDictionarySupportAlterTableTest.scala ---
    @@ -1161,6 +1159,126 @@ class LocalDictionarySupportAlterTableTest extends QueryTest with BeforeAndAfter
         }
       }
     
    +  test("test alter for local dictionary for complex columns when local dictionary exclude is defined _001") {
    +    sql("drop table if exists local1")
    +    sql(
    +      """
    +        | CREATE TABLE local1(id int, name string,city string, st array<struct<si:int,sd:string>>)
    +        | STORED BY 'org.apache.carbondata.format'
    +        | tblproperties('long_string_columns'='name','local_dictionary_enable'='true')
    +      """.stripMargin)
    +    sql("alter table local1 set tblproperties('local_dictionary_exclude'='st,name')")
    +    val descLoc = sql("describe formatted local1").collect
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Threshold")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("10000"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Enabled")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("true"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Exclude")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("st.val.sd,name"))
    +    }
    +  }
    +
    +  test("test alter for local dictionary for complex columns when local dictionary exclude is defined _002") {
    +    sql("drop table if exists local1")
    +    sql(
    +      """
    +        | CREATE TABLE local1(id int, name string,city string, st array<struct<si:int,sd:string>>,f string,g int,h string)
    +        | STORED BY 'org.apache.carbondata.format'
    +        | tblproperties('long_string_columns'='name','local_dictionary_enable'='true','local_dictionary_include'='st')
    +      """.stripMargin)
    +    sql("alter table local1 unset tblproperties('local_dictionary_include')")
    +    sql("alter table local1 set tblproperties('local_dictionary_exclude'='st,name,h')")
    +    val descLoc = sql("describe formatted local1").collect
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Threshold")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("10000"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Enabled")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("true"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Exclude")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("h,st.val.sd,name"))
    --- End diff --
   
    Local Dictionary Exclude will display only no-dictionary string/varchar columns. Those are the only child columns which will be displayed.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2469#discussion_r201267613
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/localdictionary/LocalDictionarySupportAlterTableTest.scala ---
    @@ -1161,6 +1159,126 @@ class LocalDictionarySupportAlterTableTest extends QueryTest with BeforeAndAfter
         }
       }
     
    +  test("test alter for local dictionary for complex columns when local dictionary exclude is defined _001") {
    +    sql("drop table if exists local1")
    +    sql(
    +      """
    +        | CREATE TABLE local1(id int, name string,city string, st array<struct<si:int,sd:string>>)
    +        | STORED BY 'org.apache.carbondata.format'
    +        | tblproperties('long_string_columns'='name','local_dictionary_enable'='true')
    +      """.stripMargin)
    +    sql("alter table local1 set tblproperties('local_dictionary_exclude'='st,name')")
    +    val descLoc = sql("describe formatted local1").collect
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Threshold")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("10000"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Enabled")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("true"))
    +    }
    +    descLoc.find(_.get(0).toString.contains("Local Dictionary Exclude")) match {
    +      case Some(row) => assert(row.get(1).toString.contains("st.val.sd,name"))
    --- End diff --
   
    si is int and Local Dictionary Include/Exclude will display only no-dictionary string/varchar columns


---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2469
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5772/



---

[qiuchenjian-2]

Github user praveenmeenakshi56 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---