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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
