Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3726: Add support for Kudu-specific column options ......................................................................
Patch Set 1: (25 comments) Still need to address the parser changes wrt the use of DEFAULT but I am sending a new patch that addresses most of the comments. http://gerrit.cloudera.org:8080/#/c/5026/1/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 79: enum TEncoding { > TColumnEncoding Done http://gerrit.cloudera.org:8080/#/c/5026/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 1286: comment_val ::= > Is a comment mandatory anywhere? If not, remove this. This was required to resolve conflicts in column options. I'll see if we can avoid this. Line 1434: IDENT:col_name type_def:type column_options_map:options > Neat solution! I like it. Done Line 1474: opt_is_primary_key_val ::= > these productions here are not really "opt" because empty is not accepted Done 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-spec Done Line 75: private Boolean isNullable_; > Why do we need to distinguish the not specified case? We seem to be assumin Well the user may set NULL, NOT NULL and not specify anything which means that Kudu will set the default behavior. Line 77: // Encoding for this column (used in Kudu); set in the analysis. > Are these really set in analysis? Yes, so the parser sets the string val and then during the analysis we convert it into an actual encoding val while checking if the specified value is valid or not. Line 95: Preconditions.checkNotNull(option.getValue()); > redundant with the instanceof check because "null instanceof anything" is f Good point. Removed. Line 160: public boolean hasKuduSpecificOptions() { > hasKuduOptions() seems sufficient Done Line 199: throw new AnalysisException("Primary key columns can't have NULL values: " + > cannot be nullable Done Line 203: if (!Strings.isNullOrEmpty(encodingVal_)) { > empty is an error right? (same in other places below) Yeah, it should never pass the parser. Changed the conditions. 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 de As is today, columns are non-nullable by default but the check here simply states that if you set the column as non-nullable you can't specify a null default value which is kind of obvious and I'd be surprised to see a different behavior. In any case, we can remove it if you think this is not the case. Line 244: throw new AnalysisException(String.format("NULL values are not allowed for " + > Default value of NULL not allowed on non-nullable column: ? Done Line 255: defaultValue_ = LiteralExpr.create(castLiteral, analyzer.getQueryCtx()); > Do we need this? We might get a different type from LiteralExpr.create() We already guarantee that the type of the default value is castable to the type of the column but we also need to ensure it has the exact type before we set the value in the catalog. Recall, we had the same problem with the boolean range value applied to a column of type int. Line 263: Integer val = Ints.tryParse(blockSize_.toSql()); > inspect the type instead, and then grab the value from NumericLiteral.getVa Done 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 Good point. Done 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 Done Line 696: throw new AnalysisException("Missing values for column " + > mention that the column has no default value and is not nullable Done 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? Done Line 72: "'%s': %s", defaultValue, e.getMessage())); > pass e as the cause (not just the msg) Done Line 79: colSchema.getDesiredBlockSize(), null, position); > why is comment always null? Kudu doesn't store comments so even if you specify comments and store them in hms, the next time column metadata are loaded from Kudu and override the HMS entries, the comments will be deleted. I believe we have a JIRA for that. Line 83: throws ImpalaRuntimeException { > where is the IRE thrown? KuduUtil.fromThrift() for encodings and compression 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 thi Actually there was a bug that was looming here. If avroCol.getComment is null then the constructor will throw an IllegalStateException. We need a check before populating the option and we can't put that in the old constructor because the call to the other constructor needs to be the first operation. 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? Done 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 Done -- 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
