[GitHub] carbondata pull request #2570: [wip]disable local dictionary by default

2018-07-30 Thread KanakaKumar 
Github user KanakaKumar commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/2570#discussion_r206146016
  
--- Diff: 
core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ---
@@ -929,7 +929,7 @@
   /**
* default value for local dictionary generation
*/
-  public static final String LOCAL_DICTIONARY_ENABLE_DEFAULT = "true";
--- End diff --

Please correct the documentation also as default value as false


---

[GitHub] carbondata pull request #2570: [wip]disable local dictionary by default

2018-07-27 Thread praveenmeenakshi56 
Github user praveenmeenakshi56 commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/2570#discussion_r205687209
  
--- Diff: 
integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/localdictionary/LocalDictionarySupportCreateTableTest.scala
 ---
@@ -1575,7 +1571,7 @@ class LocalDictionarySupportCreateTableTest extends 
QueryTest with BeforeAndAfte
 val descLoc = sql("describe formatted local1").collect
 
 descLoc.find(_.get(0).toString.contains("Local Dictionary Enabled")) 
match {
-  case Some(row) => assert(row.get(1).toString.contains("true"))
+  case Some(row) => assert(row.get(1).toString.contains("false"))
--- End diff --

If local dictionary is disabled here, it will be the same as previous test 
case. Can you make it true and add other properties like 
local_dictionary_threshold or local_dictionary_include, etc.


---

[GitHub] carbondata pull request #2570: [wip]disable local dictionary by default

2018-07-27 Thread praveenmeenakshi56 
Github user praveenmeenakshi56 commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/2570#discussion_r205686883
  
--- Diff: 
integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/localdictionary/LocalDictionarySupportCreateTableTest.scala
 ---
@@ -1554,7 +1550,7 @@ class LocalDictionarySupportCreateTableTest extends 
QueryTest with BeforeAndAfte
 val descLoc = sql("describe formatted local1").collect
 
 descLoc.find(_.get(0).toString.contains("Local Dictionary Enabled")) 
match {
-  case Some(row) => assert(row.get(1).toString.contains("true"))
+  case Some(row) => assert(row.get(1).toString.contains("false"))
--- End diff --

If local dictionary is disabled here, it will be the same as previous test 
case. Can you make it true and add other properties like 
local_dictionary_threshold or local_dictionary_include, etc. 


---

[GitHub] carbondata pull request #2570: [wip]disable local dictionary by default

2018-07-27 Thread praveenmeenakshi56 
Github user praveenmeenakshi56 commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/2570#discussion_r205684804
  
--- Diff: 
integration/spark-common-cluster-test/src/test/scala/org/apache/carbondata/cluster/sdv/generated/CreateTableWithLocalDictionaryTestCase.scala
 ---
@@ -28,7 +28,7 @@ class CreateTableWithLocalDictionaryTestCase extends 
QueryTest with BeforeAndAft
 sql("DROP TABLE IF EXISTS LOCAL1")
   }
 
-  test("test local dictionary default configuration") {
+   test("test local dictionary default configuration") {
--- End diff --

remove the extra space.


---

[GitHub] carbondata pull request #2570: [wip]disable local dictionary by default

2018-07-27 Thread akashrn5 
GitHub user akashrn5 opened a pull request:

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

[wip]disable local dictionary by default

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/akashrn5/incubator-carbondata 
local_default_false

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

https://github.com/apache/carbondata/pull/2570.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 #2570


commit e0c10c5d3bf837278518a259618fe7dee8817564
Author: akashrn5 
Date:   2018-07-27T07:01:48Z

disable local dictionary by default




---