[GitHub] [carbondata] jackylk opened a new pull request #3595: [WIP] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage

[GitBox]

jackylk opened a new pull request #3595: [WIP] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595
 
 
   Why is this PR needed?
   After Global Dictionary feature is deprecated, we can refactor the Dict/NoDict usage. This PR is one of the effort for this refactory.
   
   What changes were proposed in this PR?
   This PR remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
   
   Does this PR introduce any user interface change?
   No
   
   Is any new testcase added?
   No

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3595: [WIP] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#issuecomment-579394148
 
 
   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1785/
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3595: [WIP] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#issuecomment-579685385
 
 
   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1788/
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3595: [WIP] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#issuecomment-579718709
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1789/
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372821631
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datastore/TableSpec.java
 ##########
 @@ -103,7 +102,7 @@ private void addDimensions(List<CarbonDimension> dimensions) {
         } else {
           noSortNoDictDimSpec.add(spec);
         }
-      } else if (dimension.isDirectDictionaryEncoding()) {
 
 Review comment:
   CarbonDimension has isGlobalDictionaryEncoding and isDirectDictionaryEncoding, please remove both the methods and handle the callers

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372822576
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/metadata/converter/ThriftWrapperSchemaConverterImpl.java
 ##########
 @@ -118,7 +118,7 @@
         return org.apache.carbondata.format.Encoding.DIRECT_COMPRESS_VARCHAR;
       case BIT_PACKED:
         return org.apache.carbondata.format.Encoding.BIT_PACKED;
-      case DIRECT_DICTIONARY:
+      case  DIRECT_DICTIONARY:
 
 Review comment:
   revert this

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372821631
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datastore/TableSpec.java
 ##########
 @@ -103,7 +102,7 @@ private void addDimensions(List<CarbonDimension> dimensions) {
         } else {
           noSortNoDictDimSpec.add(spec);
         }
-      } else if (dimension.isDirectDictionaryEncoding()) {
 
 Review comment:
   CarbonDimension has isGlobalDictionaryEncoding and isDirectDictionaryEncoding, please remove both the methods and handle the callers

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372825333
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java
 ##########
 @@ -363,14 +363,14 @@ private void fillDimensionsAndMeasuresForTables(TableSchema tableSchema) {
           if (!columnSchema.isInvisible() && columnSchema.isSortColumn()) {
             this.numberOfSortColumns++;
           }
-          if (!columnSchema.getEncodingList().contains(Encoding.DICTIONARY)) {
+          if (columnSchema.getDataType() != DataTypes.DATE) {
 
 Review comment:
   I think all the places we can remove below code also.
   encodings.add(Encoding.DIRECT_DICTIONARY);
         encodings.add(Encoding.DICTIONARY);
   
   Also can remove the carbonColumn.hasEncoding(Encoding.DIRECT_DICTIONARY) check from all the places.
   
   so, can lookup the usages of Encoding.DIRECT_DICTIONARY, we can remove everywhere other than thrift to maintain the compatibility

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372825697
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java
 ##########
 @@ -363,14 +363,14 @@ private void fillDimensionsAndMeasuresForTables(TableSchema tableSchema) {
           if (!columnSchema.isInvisible() && columnSchema.isSortColumn()) {
             this.numberOfSortColumns++;
           }
-          if (!columnSchema.getEncodingList().contains(Encoding.DICTIONARY)) {
+          if (columnSchema.getDataType() != DataTypes.DATE) {
 
 Review comment:
   similar comment for Encoding.DICTIONARY also

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372827270
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datastore/TableSpec.java
 ##########
 @@ -103,7 +102,7 @@ private void addDimensions(List<CarbonDimension> dimensions) {
         } else {
           noSortNoDictDimSpec.add(spec);
         }
-      } else if (dimension.isDirectDictionaryEncoding()) {
+      } else if (dimension.getDataType() == DataTypes.DATE) {
         spec = new DimensionSpec(ColumnType.DIRECT_DICTIONARY, dimension, dictActualPosition++);
 
 Review comment:
   ColumnType.DIRECT_DICTIONARY, we are still keeping ? we can refactor this also

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on issue #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#issuecomment-580155563
 
 
   Also I hope Encoding.Dictionary is not used for local dictionary, I will cross check. May be need to test once compatibility with old store also.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372836145
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java
 ##########
 @@ -363,14 +363,14 @@ private void fillDimensionsAndMeasuresForTables(TableSchema tableSchema) {
           if (!columnSchema.isInvisible() && columnSchema.isSortColumn()) {
             this.numberOfSortColumns++;
           }
-          if (!columnSchema.getEncodingList().contains(Encoding.DICTIONARY)) {
+          if (columnSchema.getDataType() != DataTypes.DATE) {
 
 Review comment:
   Following need to be removed later in another PR
   encodings.add(Encoding.DIRECT_DICTIONARY);
   encodings.add(Encoding.DICTIONARY);
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372836461
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datastore/TableSpec.java
 ##########
 @@ -103,7 +102,7 @@ private void addDimensions(List<CarbonDimension> dimensions) {
         } else {
           noSortNoDictDimSpec.add(spec);
         }
-      } else if (dimension.isDirectDictionaryEncoding()) {
+      } else if (dimension.getDataType() == DataTypes.DATE) {
         spec = new DimensionSpec(ColumnType.DIRECT_DICTIONARY, dimension, dictActualPosition++);
 
 Review comment:
   I will refactor ColumnType in another PR

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372836461
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datastore/TableSpec.java
 ##########
 @@ -103,7 +102,7 @@ private void addDimensions(List<CarbonDimension> dimensions) {
         } else {
           noSortNoDictDimSpec.add(spec);
         }
-      } else if (dimension.isDirectDictionaryEncoding()) {
+      } else if (dimension.getDataType() == DataTypes.DATE) {
         spec = new DimensionSpec(ColumnType.DIRECT_DICTIONARY, dimension, dictActualPosition++);
 
 Review comment:
   I will refactor ColumnType in another PR

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372838310
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datastore/TableSpec.java
 ##########
 @@ -103,7 +102,7 @@ private void addDimensions(List<CarbonDimension> dimensions) {
         } else {
           noSortNoDictDimSpec.add(spec);
         }
-      } else if (dimension.isDirectDictionaryEncoding()) {
+      } else if (dimension.getDataType() == DataTypes.DATE) {
         spec = new DimensionSpec(ColumnType.DIRECT_DICTIONARY, dimension, dictActualPosition++);
 
 Review comment:
   This is not possible in this PR since DATE is still a direct dictionary, need to do in another PR

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r372836145
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java
 ##########
 @@ -363,14 +363,14 @@ private void fillDimensionsAndMeasuresForTables(TableSchema tableSchema) {
           if (!columnSchema.isInvisible() && columnSchema.isSortColumn()) {
             this.numberOfSortColumns++;
           }
-          if (!columnSchema.getEncodingList().contains(Encoding.DICTIONARY)) {
+          if (columnSchema.getDataType() != DataTypes.DATE) {
 
 Review comment:
   Following need to be removed later in another PR
   encodings.add(Encoding.DIRECT_DICTIONARY);
   encodings.add(Encoding.DICTIONARY);
   
   This is not possible in this PR since DATE is still a direct dictionary, need to do in another PR
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#issuecomment-580196993
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1804/
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r373309450
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/metadata/converter/ThriftWrapperSchemaConverterImpl.java
 ##########
 @@ -118,7 +118,7 @@
         return org.apache.carbondata.format.Encoding.DIRECT_COMPRESS_VARCHAR;
       case BIT_PACKED:
         return org.apache.carbondata.format.Encoding.BIT_PACKED;
-      case DIRECT_DICTIONARY:
+      case  DIRECT_DICTIONARY:
 
 Review comment:
   fixed

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

xubo245 commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r373480977
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/scan/collector/impl/RestructureBasedRawResultCollector.java
 ##########
 @@ -88,7 +88,7 @@ private void initRestructuredKeyGenerator() {
           // partitioner index will be 1 every column will be in columnar format
           updatedDimensionPartitioner.add(1);
           // for direct dictionary 4 bytes need to be allocated else 1
-          if (queryDimensions[i].getDimension().hasEncoding(Encoding.DIRECT_DICTIONARY)) {
+          if (queryDimensions[i].getDimension().getDataType() == DataTypes.DATE) {
 
 Review comment:
   it will be alway true because it the  same with line 76, it should be optimized.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

[GitBox]

xubo245 commented on a change in pull request #3595: [CARBONDATA-3674] remove Encoding.DICTIONARY and Encoding.DIRECT_DICTIONARY usage
URL: https://github.com/apache/carbondata/pull/3595#discussion_r373492830
 
 

 ##########
 File path: processing/src/main/java/org/apache/carbondata/processing/datatypes/ArrayDataType.java
 ##########
 @@ -64,7 +64,7 @@
   /**
    * Dictionary column
 
 Review comment:
   Please change the describe

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services