[GitHub] carbondata pull request #2605: [CARBONDATA-2585] Fix local dictionary for bo...

[qiuchenjian-2]

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 <akashnilugal@...>
Date:   2018-08-02T14:50:48Z

    fix local dictionary for both table level and system level property based on priority

----


---

[qiuchenjian-2]

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


---

[qiuchenjian-2]

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


---

[qiuchenjian-2]

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


---

[qiuchenjian-2]

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"


---

[qiuchenjian-2]

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


---

[qiuchenjian-2]

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


---

[qiuchenjian-2]

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/



---

[qiuchenjian-2]

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/



---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

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


---

[qiuchenjian-2]

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/



---

[qiuchenjian-2]

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/



---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

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


---

[qiuchenjian-2]

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/



---

[qiuchenjian-2]

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/



---

[qiuchenjian-2]

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


---

[qiuchenjian-2]

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


---

[qiuchenjian-2]

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?


---

[qiuchenjian-2]

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?


---

[qiuchenjian-2]

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/



---