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-3726: Add support for Kudu-specific column options
Date Thu, 10 Nov 2016 07:36:30 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-3726: Add support for Kudu-specific column options
......................................................................


Patch Set 1:

(3 comments)

Looks pretty good, will continue tomorrow.

http://gerrit.cloudera.org:8080/#/c/5026/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

Line 250:   KW_DATE, KW_DATETIME, KW_DECIMAL, KW_DEFAULT, KW_DELETE, KW_DELIMITED, KW_DESC,
> It's really annoying but I think we should definitely consider making it a 
Agree. Using "default" as the default DB was a very bad idea and now we have to pay for it.
Not sure if we can change the name of the default database, but we might be able to do that
together with making the keyword change in a compatibility breaking release.


Line 1466:   | opt_default_val:default_val
> Changed it to using IDENT. It does remove some flexibility wrt where NOT NU
Didn't follow the last part, maybe you can explain it to me tomorrow :)


http://gerrit.cloudera.org:8080/#/c/5026/1/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java:

Line 255:         defaultValue_ = LiteralExpr.create(castLiteral, analyzer.getQueryCtx());
> That's not a problem because both tinyint and int are stored in thrift as i
Makes sense, thanks for refreshing my memory.
I'm not questioning the approach here, just questioning whether we should have a final cast
after this line.

I'm fine with leaving it for now, but we are relying on the thrift i64. If in the future we
support other partition types like float or decimal, we will have to add another cast here.


-- 
To view, visit http://gerrit.cloudera.org:8080/5026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I727b9ae1b7b2387db752b58081398dd3f3449c02
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message