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 02:23:27 GMT
Alex Behm has posted comments on this change.

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


Patch Set 1:

(29 comments)

http://gerrit.cloudera.org:8080/#/c/5026/1/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:

Line 79: enum TEncoding {
TColumnEncoding


Line 212:   16: optional i32 block_size
i64? or what's a typical value for the block size?


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,
Making "default" a keyword sounds bad because of the "default" database. I can see how it
might break existing applications. Let's think a little and see if we can avoid it.


Line 1286: comment_val ::=
Is a comment mandatory anywhere? If not, remove this.


Line 1434:   IDENT:col_name type_def:type column_options_map:options
Neat solution! I like it.


Line 1466:   | opt_default_val:default_val
I think we might be able to get away with the usual hack for not making default a keyword
here. I don't think a leading IDENT will conflict. Can you try it?


Line 1474: opt_is_primary_key_val ::=
these productions here are not really "opt" because empty is not accepted

the optional part is having column options or not (handled at the top-level in column_def)


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 72:   private boolean isPrimaryKey_;
Instead of writing "(used in Kudu)" everywhere, can you group the Kudu-specific options by
a leading comment? For example, you could move comment_ to the top and then have a leading
comment that all following options are Kudu specific.


Line 75:   private Boolean isNullable_;
Why do we need to distinguish the not specified case? We seem to be assuming that the default
value for this is "false". See e.g. L242


Line 77:   // Encoding for this column (used in Kudu); set in the analysis.
Are these really set in analysis?


Line 95:           Preconditions.checkNotNull(option.getValue());
redundant with the instanceof check because "null instanceof anything" is false.


Line 160:   public boolean hasKuduSpecificOptions() {
hasKuduOptions() seems sufficient


Line 199:       throw new AnalysisException("Primary key columns can't have NULL values: "
+
cannot be nullable


Line 203:     if (!Strings.isNullOrEmpty(encodingVal_)) {
empty is an error right? (same in other places below)


Line 242:       if (defaultValue_.getType().isNull() && ((isNullable_ != null &&
!isNullable_)
Should we defer this check to Kudu? I'm if whether we can assume that by default all columns
are nullable or whether Kudu maybe has a config to specify the default nullability.


Line 244:         throw new AnalysisException(String.format("NULL values are not allowed for
" +
Default value of NULL not allowed on non-nullable column: ?


Line 255:         defaultValue_ = LiteralExpr.create(castLiteral, analyzer.getQueryCtx());
Do we need this? We might get a different type from LiteralExpr.create()


Line 263:       Integer val = Ints.tryParse(blockSize_.toSql());
inspect the type instead, and then grab the value from NumericLiteral.getValue().intValue()?


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

Line 152:             Collections.<ColumnDef.Option, Object>emptyMap());
instead of changing all callers, how about adding a second ColumnDef c'tor without the map
that calls the other c'tor passing an empty map


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

Line 134:   // For Kudu tables (INSERT and UPSERT operaitions), it will contain one Expr per
column
typo: operations


Line 696:               throw new AnalysisException("Missing values for column " +
mention that the column has no default value and is not nullable


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

Line 47:   private LiteralExpr defaultValue_;
final?


Line 72:             "'%s': %s", defaultValue, e.getMessage()));
pass e as the cause (not just the msg)


Line 79:         colSchema.getDesiredBlockSize(), null, position);
why is comment always null?


Line 83:       throws ImpalaRuntimeException {
where is the IRE thrown?


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

Line 144:         ColumnDef reconciledColDef = new ColumnDef(
Maybe also keep the old c'tor with only comment? I think you can create this map in that c'tor
and call the other c'tor.


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

Line 264:   public static TColumn toTKuduColumnHelper(TColumn column, boolean isKey,
setColumnOptions() or something more descriptive?


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

Line 1958:               AnalyzesOk(String.format("create table tab (x int primary key " +
nice


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

Line 934:     ParsesOk("select a from `default`.`t`");
:(


-- 
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