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 <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-HasComments: Yes
