impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5286/IMPALA-5283: Kudu column name case cleanup
Date Wed, 14 Jun 2017 23:28:27 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-5286/IMPALA-5283: Kudu column name case cleanup
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6902/2/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:

Line 194:   1: required string columnName
This should be the lower case column name.

For the original Kudu column name, we should add a new field at the bottom where the Kudu-specific
stuff is.


Line 336:   // Column names, in Kudu case.
Shouldn't everything be in Impala case by default? Based on your Patch Set 3, I don't this
and the changes below in this file are needed.


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:     List<String> colNames = Lists.<String>newArrayList();
These changes were not needed in your Patch Set 3 version. Can we remove this?


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:   // Primary key column names, in Impala case.
Impala case -> lower case (fix here and elsewhere)


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message