[GitHub] [carbondata] VenuReddy2103 opened a new pull request #3717: [HOTFIX]Old store read compatibility issue for Index handler column

[GitBox]

VenuReddy2103 opened a new pull request #3717: [HOTFIX]Old store read compatibility issue for Index handler column
URL: https://github.com/apache/carbondata/pull/3717
 
 
   
    ### Why is this PR needed?
    Table created in older version do not have indexColumn property in columnSchema. Newer versions have indexColumn property in columnSchema with default value as false. Due to this, when Old store is read, index handler column which is hidden to user is also treated as normal schema column and can perform operations like alter table on the index column.
   
    ### What changes were proposed in this PR?
   During schema read, if the index_handler property is present, check if column name matches to index handler name and set indexColumn property value to true.
       
    ### 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 #3717: [HOTFIX]Old store read compatibility issue for Index handler column
URL: https://github.com/apache/carbondata/pull/3717#issuecomment-614763086
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2756/
   

----------------------------------------------------------------
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 issue #3717: [HOTFIX]Old store read compatibility issue for Index handler column
URL: https://github.com/apache/carbondata/pull/3717#issuecomment-615539339
 
 
   Isn't indexColumn property in columnSchema is optional, why need to add this check?

----------------------------------------------------------------
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 edited a comment on issue #3717: [HOTFIX]Old store read compatibility issue for Index handler column
URL: https://github.com/apache/carbondata/pull/3717#issuecomment-615539339
 
 
   Isn't indexColumn property in columnSchema is optional, why need to add this check, or this check is in wrong place

----------------------------------------------------------------
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 #3717: [HOTFIX]Old store read compatibility issue for Index handler column
URL: https://github.com/apache/carbondata/pull/3717#discussion_r410557172
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/metadata/converter/ThriftWrapperSchemaConverterImpl.java
 ##########
 @@ -605,6 +605,14 @@ public TableSchema fromExternalToWrapperTableSchema(
           .setLocalDictColumnsToWrapperSchema(listOfColumns, externalTableSchema.tableProperties,
               externalTableSchema.tableProperties
                   .get(CarbonCommonConstants.LOCAL_DICTIONARY_ENABLE));
+      String handler = externalTableSchema.tableProperties.get(CarbonCommonConstants.INDEX_HANDLER);
+      if (handler != null) {
+        for (ColumnSchema columnSchema : listOfColumns) {
+          if (handler.equalsIgnoreCase(columnSchema.getColumnName())) {
 
 Review comment:
   Why are you checking column name against handler?
   Either of these is not a proper name

----------------------------------------------------------------
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 edited a comment on issue #3717: [HOTFIX]Old store read compatibility issue for Index handler column
URL: https://github.com/apache/carbondata/pull/3717#issuecomment-615539339
 
 
   Isn't indexColumn property in columnSchema is optional, why need to add this check, or this check is in wrong place (should have check when this property is being used)

----------------------------------------------------------------
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]

VenuReddy2103 commented on a change in pull request #3717: [HOTFIX]Old store read compatibility issue for Index handler column
URL: https://github.com/apache/carbondata/pull/3717#discussion_r410727838
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/metadata/converter/ThriftWrapperSchemaConverterImpl.java
 ##########
 @@ -605,6 +605,14 @@ public TableSchema fromExternalToWrapperTableSchema(
           .setLocalDictColumnsToWrapperSchema(listOfColumns, externalTableSchema.tableProperties,
               externalTableSchema.tableProperties
                   .get(CarbonCommonConstants.LOCAL_DICTIONARY_ENABLE));
+      String handler = externalTableSchema.tableProperties.get(CarbonCommonConstants.INDEX_HANDLER);
+      if (handler != null) {
+        for (ColumnSchema columnSchema : listOfColumns) {
+          if (handler.equalsIgnoreCase(columnSchema.getColumnName())) {
 
 Review comment:
   index column was created with index_handler property value as its name during create table in `processIndexProperty` method. So matching handler value against the column name.

----------------------------------------------------------------
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]

VenuReddy2103 commented on issue #3717: [HOTFIX]Old store read compatibility issue for Index handler column
URL: https://github.com/apache/carbondata/pull/3717#issuecomment-615913560
 
 
   > Isn't indexColumn property in columnSchema is optional, why need to add this check, or this check is in wrong place (should have check when this property is being used)
   
   Yes, It is optional. But, Table created in older version do not have indexColumn property in columnSchema.(schema.thrift). We had considered columnSchema.schemaOrdinal being -1 as index column instead of defining a new property.
           But in  the later version, we have defined indexColumn property in columnSchema with default value as false. Due to this, when Old store is read, index handler column which is hidden to user is also treated as normal schema column(since indexColumn property default value is false) and user can perform operations like alter table on the index column and create inconsistency.
           Thats why, when we read the the schema, we update the indexColumn field in columnSchema to true if it is index column.
   

----------------------------------------------------------------
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