[GitHub] carbondata pull request #2704: [HOTFIX] Old stores cannot read with new tabl...

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2704#discussion_r217019741
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/executor/impl/AbstractQueryExecutor.java ---
    @@ -476,7 +476,11 @@ private BlockExecutionInfo getBlockExecutionInfoForBlock(QueryModel queryModel,
         blockExecutionInfo
             .setTotalNumberDimensionToRead(
                 segmentProperties.getDimensionOrdinalToChunkMapping().size());
    -    blockExecutionInfo.setPrefetchBlocklet(!queryModel.isReadPageByPage());
    +    if (queryModel.isReadPageByPage()) {
    --- End diff --
   
    Updated description


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2704#discussion_r217020324
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/CarbonMetadata.java ---
    @@ -143,7 +143,7 @@ public CarbonDimension getCarbonDimensionBasedOnColIdentifier(CarbonTable carbon
         List<CarbonDimension> listOfCarbonDims =
             carbonTable.getDimensionByTableName(carbonTable.getTableName());
         for (CarbonDimension dimension : listOfCarbonDims) {
    -      if (dimension.getColumnId().equals(columnIdentifier)) {
    +      if (dimension.getColumnId().equalsIgnoreCase(columnIdentifier)) {
    --- End diff --
   
    this is the case for non-transactional flow. It uses column name for columnid as well. It is the safest way to do the check and there would not be any side effects on it,


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2704#discussion_r217021511
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java ---
    @@ -165,14 +165,15 @@ private static boolean isColumnMatches(boolean isTransactionalTable,
         // column ID but can have same column name
         if (tableColumn.getDataType().isComplexType() && !(tableColumn.getDataType().getId()
             == DataTypes.ARRAY_TYPE_ID)) {
    -      if (tableColumn.getColumnId().equals(queryColumn.getColumnId())) {
    +      if (tableColumn.getColumnId().equalsIgnoreCase(queryColumn.getColumnId())) {
             return true;
           } else {
             return isColumnMatchesStruct(tableColumn, queryColumn);
           }
         } else {
    -      return (tableColumn.getColumnId().equals(queryColumn.getColumnId()) || (!isTransactionalTable
    -          && tableColumn.getColName().equals(queryColumn.getColName())));
    +      return (tableColumn.getColumnId().equalsIgnoreCase(queryColumn.getColumnId()) || (
    --- End diff --
   
    There would not be restructure for non transactional table. And dso the case sensitivity check only for safety.


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2704#discussion_r217021636
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java ---
    @@ -165,14 +165,15 @@ private static boolean isColumnMatches(boolean isTransactionalTable,
         // column ID but can have same column name
         if (tableColumn.getDataType().isComplexType() && !(tableColumn.getDataType().getId()
             == DataTypes.ARRAY_TYPE_ID)) {
    -      if (tableColumn.getColumnId().equals(queryColumn.getColumnId())) {
    +      if (tableColumn.getColumnId().equalsIgnoreCase(queryColumn.getColumnId())) {
    --- End diff --
   
    same as above comment


---

[qiuchenjian-2]

Github user KanakaKumar commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2704#discussion_r217021955
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java ---
    @@ -165,14 +165,15 @@ private static boolean isColumnMatches(boolean isTransactionalTable,
         // column ID but can have same column name
         if (tableColumn.getDataType().isComplexType() && !(tableColumn.getDataType().getId()
             == DataTypes.ARRAY_TYPE_ID)) {
    -      if (tableColumn.getColumnId().equals(queryColumn.getColumnId())) {
    +      if (tableColumn.getColumnId().equalsIgnoreCase(queryColumn.getColumnId())) {
             return true;
           } else {
             return isColumnMatchesStruct(tableColumn, queryColumn);
           }
         } else {
    -      return (tableColumn.getColumnId().equals(queryColumn.getColumnId()) || (!isTransactionalTable
    -          && tableColumn.getColName().equals(queryColumn.getColName())));
    +      return (tableColumn.getColumnId().equalsIgnoreCase(queryColumn.getColumnId()) || (
    --- End diff --
   
    OK


---

[qiuchenjian-2]

Github user KanakaKumar commented on the issue:

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


---

[qiuchenjian-2]

Github user kumarvishal09 commented on the issue:

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


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---