impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Ribeiro Alves (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) Review Only: Kudu Impala integration
Date Fri, 11 Mar 2016 06:27:18 GMT
David Ribeiro Alves has posted comments on this change.

Change subject: Review Only: Kudu Impala integration

Patch Set 7:

File fe/src/main/cup/sql-parser.cup:

Line 233: // List of keywords. Please keep them sorted alphabetically.
> please add

Line 237:   KW_BUCKETS, KW_BY,
> some long lines, and some very short ones. try to fill them up.

Line 717:   KW_DELETE opt_ignore:ignore dotted_path:target_table  where_clause:where
> what's wrong with opt_kw_from instead of duplicating the production?
I got:
** Shift/Reduce conflict found in state #1207
between opt_kw_from ::= (*) 
and dotted_path ::= (*) IDENT 
under symbol IDENT
Resolved in favor of shifting.

Talked to Alex, he suggested we could do it like this.

Line 977:   // Create partitioned tables with CTAS statement. We need an own production
> "a separate"

Line 980:   // follow the PARTITION feature of insert from select statements.
> orphaned sentence

Line 982:   table_name:table
> short line

Line 989:     // Initialize with empty List of columns. The columns will be added by the query
> lower case

Line 2802: ident_or_keyword ::=
> did you add your new keywords?
yeah, otherwise it wouldn't work, right? (see BUCKETS, IGNORE, etc)
File fe/src/main/java/com/cloudera/impala/analysis/

Line 116:    * predicate into literals, if possible.
> add "returns null if this is not a slotref comparison"
File fe/src/main/java/com/cloudera/impala/analysis/

Line 140:             throw new AnalysisException(String.format("Split row value is not supported:
%s "
> long line

Line 141:                 + "(Type: %s) is not supported.", expr.getStringValue(), expr.getType().toSql()));
> remove trailing 'is not supported'.
File fe/src/main/java/com/cloudera/impala/analysis/

Line 669:    * logic from DataSink to TableSink.
> didn't you do that?
right, missed that. Done
File fe/src/main/java/com/cloudera/impala/analysis/

Line 156:     if (sourceStmt_ == null) {
> single line

Line 166:     if (sourceStmt_ != null) {
> single line

Line 251:             format("Left-hand side column '%s' in assignment expression '%s=%s'
does not " +
> long line

Line 285:             c.getName(), c.getType().toSql(), rhsExpr.toSql(), rhsExpr.getType().toSql()));
> long line
File fe/src/main/java/com/cloudera/impala/analysis/

Line 84:     this.selectList_ = selectList;
> remove this.
File fe/src/main/java/com/cloudera/impala/catalog/

Line 1: // Copyright 2012 Cloudera Inc.
> could you flag what you modified since version 6, if anything?
nothing was changed, this is pretty much trunk plus the extra ctor
File fe/src/main/java/com/cloudera/impala/catalog/

Line 258:         // TODO why -1?
> good point. what is this?
couldn't figure it out. changed this to a TODO(kudu-merge) to inspect post merge.
File fe/src/main/java/com/cloudera/impala/catalog/

Line 430:   public void setMetaStoreTable(org.apache.hadoop.hive.metastore.api.Table msTbl)
> precede w/ blank line
File fe/src/main/java/com/cloudera/impala/catalog/delegates/

Line 124:         for (TDistributeParam dP : distributeParams_) {
> dP -> param or something like that. dP is weird.

Line 126:             cto.addHashPartitions(dP.getBy_hash_param().getColumns(),
> also checkstate that by_range_param isn't set
File fe/src/main/java/com/cloudera/impala/planner/

Line 68:   // Indexes for the set of hosts that will be used for the query.
> explain what it indexes into

Line 178:     cardinality_ = Math.max(Math.max(1, cardinality_), kuduTable_.getNumRows());
> needs to be min(max(1, card), #rows).
File fe/src/main/java/com/cloudera/impala/planner/

Line 43:    * Returns an output sink appropriate for writing to the given table.
> explain that not all ops are supported and params can be null, as dictated 
actually no param can be null (lists are empty if they don't make sense) made that clear.

Line 78:    * Enum to specify the sink operation type.
> move above member vars.
File fe/src/main/java/com/cloudera/impala/util/

Line 145:   private static void setKey(PartialRow key, Type type, JsonArray array, int pos)
> key is an in/out param and needs to go to the end. or consider adding to Pa

Line 163:   private static void setKey(PartialRow key, Type type, TRangeLiteral literal, int
> same here

Line 168:         key.addBoolean(pos, literal.bool_literal);
> use getter here as well for consistency
File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test:

Line 75: # The updating a varchar col. with a value that is bigger than it's size (truncated).
> fix grammar

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I239314acbc8434ef673a3a59d2a82a0338ea5876
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: David Ribeiro Alves <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: David Ribeiro Alves <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Martin Grund <>
Gerrit-Reviewer: Silvius Rus <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

View raw message