You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@carbondata.apache.org by praveenmeenakshi56 <gi...@git.apache.org> on 2018/07/09 15:10:01 UTC
[GitHub] carbondata pull request #2469: Added fix for Local Dictionary Exclude for mu...
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 <pr...@...>
Date: 2018-07-09T15:06:36Z
Added fix for Local Dictionary Exclude for multi level complex columns
----
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by kumarvishal09 <gi...@git.apache.org>.
Github user kumarvishal09 commented on the issue:
https://github.com/apache/carbondata/pull/2469
LGTM
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by kumarvishal09 <gi...@git.apache.org>.
Github user kumarvishal09 commented on the issue:
https://github.com/apache/carbondata/pull/2469
retest this please
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by praveenmeenakshi56 <gi...@git.apache.org>.
Github user praveenmeenakshi56 commented on the issue:
https://github.com/apache/carbondata/pull/2469
retest this please
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata pull request #2469: [CARBONDATA-2712] Added fix for Local Diction...
Posted by akashrn5 <gi...@git.apache.org>.
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
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by ravipesala <gi...@git.apache.org>.
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/5771/
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by ravipesala <gi...@git.apache.org>.
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/
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata pull request #2469: [CARBONDATA-2712] Added fix for Local Diction...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/carbondata/pull/2469
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/5796/
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by praveenmeenakshi56 <gi...@git.apache.org>.
Github user praveenmeenakshi56 commented on the issue:
https://github.com/apache/carbondata/pull/2469
retest this please
---
[GitHub] carbondata pull request #2469: [CARBONDATA-2712] Added fix for Local Diction...
Posted by praveenmeenakshi56 <gi...@git.apache.org>.
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
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by ravipesala <gi...@git.apache.org>.
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/
---
[GitHub] carbondata pull request #2469: [CARBONDATA-2712] Added fix for Local Diction...
Posted by akashrn5 <gi...@git.apache.org>.
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?
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by akashrn5 <gi...@git.apache.org>.
Github user akashrn5 commented on the issue:
https://github.com/apache/carbondata/pull/2469
LGTM
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata pull request #2469: [CARBONDATA-2712] Added fix for Local Diction...
Posted by akashrn5 <gi...@git.apache.org>.
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
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by ravipesala <gi...@git.apache.org>.
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/
---
[GitHub] carbondata pull request #2469: [CARBONDATA-2712] Added fix for Local Diction...
Posted by praveenmeenakshi56 <gi...@git.apache.org>.
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.
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/7021/
---
[GitHub] carbondata issue #2469: [CARBONDATA-2712] Added fix for Local Dictionary Exc...
Posted by CarbonDataQA <gi...@git.apache.org>.
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/5800/
---
[GitHub] carbondata pull request #2469: [CARBONDATA-2712] Added fix for Local Diction...
Posted by akashrn5 <gi...@git.apache.org>.
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.
---