Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-5286/IMPALA-5283: Kudu column name case cleanup ......................................................................
Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/6902/2/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 193: 1: required string columnName > It's a shame we have to expose a new concept in non-Kudu-specific structs. Done Line 194: 2: required Types.TColumnType columnType > This should be the lower case column name. Done Line 336: 2: required i32 num_partitions > Shouldn't everything be in Impala case by default? Based on your Patch Set Done http://gerrit.cloudera.org:8080/#/c/6902/3/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 371: // Column names in the casing that they appear in Kudu. > Mention that these are in an arbitrary order Done http://gerrit.cloudera.org:8080/#/c/6902/2/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 1246: | KW_HASH KW_PARTITIONS INTEGER_LITERAL:numPartitions > These changes were not needed in your Patch Set 3 version. Can we remove th Done http://gerrit.cloudera.org:8080/#/c/6902/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 959: key.equals(key.toLowerCase()), "Slot paths should be lower case: " + key); > use StringUtils.isAllLowerCase() here and elsewhere That doesn't work as isAllLowerCase considers numbers to not be lower case. http://gerrit.cloudera.org:8080/#/c/6902/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: Line 109: private String kuduMasters_; > Impala case -> lower case (fix here and elsewhere) Done http://gerrit.cloudera.org:8080/#/c/6902/3/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: Line 255: Set<String> colNames = Sets.newHashSet(); > lowerCaseColNames Done Line 258: if (colNames.contains(lowerCaseColName)) { > instead of the extra contains() you can check the return value of add() Done Line 260: String.format("Error loading Kudu table: conflicting column name '%s'", > Spell out what the problem is. "Conflicting column name" is not very clear. Done -- To view, visit http://gerrit.cloudera.org:8080/6902 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I14aba88510012174716691b9946e1c7d54d01b44 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
