impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
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:

File common/thrift/CatalogObjects.thrift:

Line 79: enum TEncoding {

Line 212:   16: optional i32 block_size
i64? or what's a typical value for the block size?
File fe/src/main/cup/sql-parser.cup:

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)
File fe/src/main/java/org/apache/impala/analysis/

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 &&
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()?
File fe/src/main/java/org/apache/impala/analysis/

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
File fe/src/main/java/org/apache/impala/analysis/

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

Line 696:               throw new AnalysisException("Missing values for column " +
mention that the column has no default value and is not nullable
File fe/src/main/java/org/apache/impala/catalog/

Line 47:   private LiteralExpr defaultValue_;

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?
File fe/src/main/java/org/apache/impala/util/

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.
File fe/src/main/java/org/apache/impala/util/

Line 264:   public static TColumn toTKuduColumnHelper(TColumn column, boolean isKey,
setColumnOptions() or something more descriptive?
File fe/src/test/java/org/apache/impala/analysis/

Line 1958:               AnalyzesOk(String.format("create table tab (x int primary key " +
File fe/src/test/java/org/apache/impala/analysis/

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I727b9ae1b7b2387db752b58081398dd3f3449c02
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-HasComments: Yes

View raw message