impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3726: Add support for Kudu-specific column options
Date Thu, 10 Nov 2016 06:02:11 GMT
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 <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