You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@carbondata.apache.org by akashrn5 <gi...@git.apache.org> on 2018/08/02 15:13:49 UTC
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
GitHub user akashrn5 opened a pull request:
https://github.com/apache/carbondata/pull/2605
[CARBONDATA-2585] Fix local dictionary for both table level and system level property based on priority
### What are the changes
Added a System level Property for local dictionary Support.
Property 'carbon.local.dictionary.enable' can be set to true/false to enable/disable local dictionary at system level.
If table level property LOCAL_DICTIONARY_ENABLE is configured, then Local Dictionary generation will be considered based on the table level property irrespective of the system level property.
If not, then the System level property 'carbon.local.dictionary.enable' value will be considered for local dictionary generation.
By default, both 'carbon.local.dictionary.enable' and LOCAL_DICTIONARY_ENABLE are false (Local Dictionary generation is disabled).
- [ ] Any interfaces changed?
NA
- [ ] Any backward compatibility impacted?
NA
- [ ] Document update required?
Yes
- [ ] 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.
Test cases are added
- [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
NA
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/akashrn5/incubator-carbondata local_system_level
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/carbondata/pull/2605.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 #2605
----
commit bda5d813ecd3c4a498c9da9d30bf0ba274ea5915
Author: akashrn5 <ak...@...>
Date: 2018-08-02T14:50:48Z
fix local dictionary for both table level and system level property based on priority
----
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2605
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7805/
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by akashrn5 <gi...@git.apache.org>.
Github user akashrn5 commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r207284617
--- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
@@ -931,6 +931,17 @@
*/
public static final String LOCAL_DICTIONARY_ENABLE_DEFAULT = "false";
+ /**
+ * System property to enable or disable local dictionary generation
+ */
+ public static final String LOCAL_DICTIONARY_SYSTEM_ENABLE = "carbon.local.dictionary.enable";
+
+ /**
+ * System property to enable or disable local dictionary generation default value
+ */
+ public static final String LOCAL_DICTIONARY_SYSTEM_ENABLE_DEFAULT = "false";
--- End diff --
ok, removed
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2605
Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6529/
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2605
Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6540/
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by akashrn5 <gi...@git.apache.org>.
Github user akashrn5 commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r208117498
--- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
@@ -931,6 +931,11 @@
*/
public static final String LOCAL_DICTIONARY_ENABLE_DEFAULT = "false";
+ /**
+ * System property to enable or disable local dictionary generation
+ */
+ public static final String LOCAL_DICTIONARY_SYSTEM_ENABLE = "carbon.local.dictionary.enable";
--- End diff --
this is done, beacuse if we have many tables to be created and all need to be local dictionary enable, then no need to give table level property for every table, can configure the system level property
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by akashrn5 <gi...@git.apache.org>.
Github user akashrn5 commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r207282883
--- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/execution/command/carbonTableSchemaCommon.scala ---
@@ -378,19 +376,65 @@ class AlterTableColumnSchemaGenerator(
}
}
+ var isLocalDictEnabledForMainTable = tableSchema.getTableProperties
+ .get(CarbonCommonConstants.LOCAL_DICTIONARY_ENABLE)
+ if (null == isLocalDictEnabledForMainTable) {
+ isLocalDictEnabledForMainTable = CarbonProperties.getInstance()
+ .getProperty(CarbonCommonConstants.LOCAL_DICTIONARY_SYSTEM_ENABLE,
+ CarbonCommonConstants.LOCAL_DICTIONARY_SYSTEM_ENABLE_DEFAULT)
+ }
+
+ val alterMutableTblProperties: scala.collection.mutable.Map[String, String] = mutable
+ .Map(alterTableModel.tableProperties.toSeq: _*)
--- End diff --
no need for null check, beacuse we are always adding sort columns
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by jackylk <gi...@git.apache.org>.
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r208111973
--- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
@@ -931,6 +931,11 @@
*/
public static final String LOCAL_DICTIONARY_ENABLE_DEFAULT = "false";
+ /**
+ * System property to enable or disable local dictionary generation
+ */
+ public static final String LOCAL_DICTIONARY_SYSTEM_ENABLE = "carbon.local.dictionary.enable";
--- End diff --
If we already have table level property, why this system level property is required? I think too many system property will make CarbonData complex to use and configure
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by jackylk <gi...@git.apache.org>.
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r208123110
--- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
@@ -931,6 +931,11 @@
*/
public static final String LOCAL_DICTIONARY_ENABLE_DEFAULT = "false";
+ /**
+ * System property to enable or disable local dictionary generation
+ */
+ public static final String LOCAL_DICTIONARY_SYSTEM_ENABLE = "carbon.local.dictionary.enable";
--- End diff --
OK, but I feel `LOCAL_DICTIONARY_SYSTEM_ENABLE ` name is not very clear, it implies disabling the dictioanry functionality, can you propose a better name?
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2605
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7816/
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by KanakaKumar <gi...@git.apache.org>.
Github user KanakaKumar commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r207278800
--- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
@@ -931,6 +931,17 @@
*/
public static final String LOCAL_DICTIONARY_ENABLE_DEFAULT = "false";
+ /**
+ * System property to enable or disable local dictionary generation
+ */
+ public static final String LOCAL_DICTIONARY_SYSTEM_ENABLE = "carbon.local.dictionary.enable";
+
+ /**
+ * System property to enable or disable local dictionary generation default value
+ */
+ public static final String LOCAL_DICTIONARY_SYSTEM_ENABLE_DEFAULT = "false";
--- End diff --
LOCAL_DICTIONARY_ENABLE_DEFAULT itself is enough for default value
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2605
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7746/
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by ravipesala <gi...@git.apache.org>.
Github user ravipesala commented on the issue:
https://github.com/apache/carbondata/pull/2605
SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/6134/
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by akashrn5 <gi...@git.apache.org>.
Github user akashrn5 commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r208157571
--- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
@@ -931,6 +931,11 @@
*/
public static final String LOCAL_DICTIONARY_ENABLE_DEFAULT = "false";
+ /**
+ * System property to enable or disable local dictionary generation
+ */
+ public static final String LOCAL_DICTIONARY_SYSTEM_ENABLE = "carbon.local.dictionary.enable";
--- End diff --
i think we can use as ENABLE_SYSTEM_LEVEL_LOCAL_DICTIONARY, is this ok?
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2605
Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6471/
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by jackylk <gi...@git.apache.org>.
Github user jackylk commented on the issue:
https://github.com/apache/carbondata/pull/2605
LGTM
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by KanakaKumar <gi...@git.apache.org>.
Github user KanakaKumar commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r207277549
--- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/execution/command/carbonTableSchemaCommon.scala ---
@@ -378,19 +376,65 @@ class AlterTableColumnSchemaGenerator(
}
}
+ var isLocalDictEnabledForMainTable = tableSchema.getTableProperties
+ .get(CarbonCommonConstants.LOCAL_DICTIONARY_ENABLE)
+ if (null == isLocalDictEnabledForMainTable) {
+ isLocalDictEnabledForMainTable = CarbonProperties.getInstance()
+ .getProperty(CarbonCommonConstants.LOCAL_DICTIONARY_SYSTEM_ENABLE,
+ CarbonCommonConstants.LOCAL_DICTIONARY_SYSTEM_ENABLE_DEFAULT)
+ }
+
+ val alterMutableTblProperties: scala.collection.mutable.Map[String, String] = mutable
+ .Map(alterTableModel.tableProperties.toSeq: _*)
--- End diff --
Please check do we need null check for alterTableModel.tableProperties
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by jackylk <gi...@git.apache.org>.
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r208186221
--- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/util/CarbonScalaUtil.scala ---
@@ -641,6 +641,78 @@ object CarbonScalaUtil {
}
}
+ /**
+ * This method validates all the child columns of complex column recursively to check whether
+ * any of the child column is of string dataType or not
+ *
+ * @param field
+ */
+ def validateChildColumnsRecursively(field: Field): Boolean = {
+ if (field.children.isDefined && null != field.children.get) {
+ field.children.get.exists { childColumn =>
+ if (childColumn.children.isDefined && null != childColumn.children.get) {
+ validateChildColumnsRecursively(childColumn)
+ } else {
+ childColumn.dataType.get.equalsIgnoreCase("string")
+ }
+ }
+ } else {
+ false
+ }
+ }
+
+ /**
+ * This method validates the local dictionary configured columns
+ *
+ * @param fields
--- End diff --
please add parameter description for all newly added parameter
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by KanakaKumar <gi...@git.apache.org>.
Github user KanakaKumar commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r207280558
--- Diff: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java ---
@@ -2987,8 +2987,12 @@ public static void setLocalDictColumnsToWrapperSchema(List<ColumnSchema> columns
if (null != localDictExcludeColumns) {
listOfDictionaryExcludeColumns = localDictExcludeColumns.trim().split("\\s*,\\s*");
}
- if (null != isLocalDictEnabledForMainTable && Boolean
- .parseBoolean(isLocalDictEnabledForMainTable)) {
+ if (null == isLocalDictEnabledForMainTable) {
--- End diff --
This change is not required if create table command is taking care of setting default value for isLocalDictEnabledForMainTable
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by ravipesala <gi...@git.apache.org>.
Github user ravipesala commented on the issue:
https://github.com/apache/carbondata/pull/2605
SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/6192/
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by xuchuanyin <gi...@git.apache.org>.
Github user xuchuanyin commented on the issue:
https://github.com/apache/carbondata/pull/2605
LGTM
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by KanakaKumar <gi...@git.apache.org>.
Github user KanakaKumar commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2605#discussion_r207280921
--- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
@@ -306,9 +306,12 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
CarbonCommonConstants.LOCAL_DICTIONARY_ENABLE_DEFAULT)
}
} else if (!isAlterFlow) {
- // if LOCAL_DICTIONARY_ENABLE is not defined, consider the default value which is true
- tableProperties.put(CarbonCommonConstants.LOCAL_DICTIONARY_ENABLE,
- CarbonCommonConstants.LOCAL_DICTIONARY_ENABLE_DEFAULT)
+ // if LOCAL_DICTIONARY_ENABLE is not defined, tru to get from system level property
--- End diff --
spell check "tyu"
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by ravipesala <gi...@git.apache.org>.
Github user ravipesala commented on the issue:
https://github.com/apache/carbondata/pull/2605
SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/6136/
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by ravipesala <gi...@git.apache.org>.
Github user ravipesala commented on the issue:
https://github.com/apache/carbondata/pull/2605
SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/6190/
---
[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/carbondata/pull/2605
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by akashrn5 <gi...@git.apache.org>.
Github user akashrn5 commented on the issue:
https://github.com/apache/carbondata/pull/2605
2.1 build has random failure, which is not related to this change
---
[GitHub] carbondata issue #2605: [CARBONDATA-2585] Fix local dictionary for both tabl...
Posted by akashrn5 <gi...@git.apache.org>.
Github user akashrn5 commented on the issue:
https://github.com/apache/carbondata/pull/2605
retest this please
---