impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Ribeiro Alves (Code Review)" <ger...@cloudera.org>
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:

(30 comments)

http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

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


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


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"
Done


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


Line 982:   table_name:table
> short line
Done


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


Line 2802: ident_or_keyword ::=
> did you add your new keywords?
yeah, otherwise it wouldn't work, right? (see BUCKETS, IGNORE, etc)


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java
File fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java:

Line 116:    * predicate into literals, if possible.
> add "returns null if this is not a slotref comparison"
Done


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/analysis/DistributeParam.java
File fe/src/main/java/com/cloudera/impala/analysis/DistributeParam.java:

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


Line 141:                 + "(Type: %s) is not supported.", expr.getStringValue(), expr.getType().toSql()));
> remove trailing 'is not supported'.
Done


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/analysis/InsertStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/InsertStmt.java:

Line 669:    * logic from DataSink to TableSink.
> didn't you do that?
right, missed that. Done


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/analysis/ModifyStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/ModifyStmt.java:

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


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


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


Line 285:             c.getName(), c.getType().toSql(), rhsExpr.toSql(), rhsExpr.getType().toSql()));
> long line
Done


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/analysis/SelectStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/SelectStmt.java:

Line 84:     this.selectList_ = selectList;
> remove this.
Done


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java:

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


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/catalog/KuduTable.java
File fe/src/main/java/com/cloudera/impala/catalog/KuduTable.java:

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.


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/catalog/Table.java
File fe/src/main/java/com/cloudera/impala/catalog/Table.java:

Line 430:   public void setMetaStoreTable(org.apache.hadoop.hive.metastore.api.Table msTbl)
{
> precede w/ blank line
Done


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/catalog/delegates/KuduDdlDelegate.java
File fe/src/main/java/com/cloudera/impala/catalog/delegates/KuduDdlDelegate.java:

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


Line 126:             cto.addHashPartitions(dP.getBy_hash_param().getColumns(),
> also checkstate that by_range_param isn't set
Done


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/planner/KuduScanNode.java
File fe/src/main/java/com/cloudera/impala/planner/KuduScanNode.java:

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


Line 178:     cardinality_ = Math.max(Math.max(1, cardinality_), kuduTable_.getNumRows());
> needs to be min(max(1, card), #rows).
Done


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/planner/TableSink.java
File fe/src/main/java/com/cloudera/impala/planner/TableSink.java:

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.
Done


http://gerrit.cloudera.org:8080/#/c/1403/7/fe/src/main/java/com/cloudera/impala/util/KuduUtil.java
File fe/src/main/java/com/cloudera/impala/util/KuduUtil.java:

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
Done


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


Line 168:         key.addBoolean(pos, literal.bool_literal);
> use getter here as well for consistency
Done


http://gerrit.cloudera.org:8080/#/c/1403/7/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
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
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/1403
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I239314acbc8434ef673a3a59d2a82a0338ea5876
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: David Ribeiro Alves <david.alves@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <david.alves@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Martin Grund <grundprinzip@gmail.com>
Gerrit-Reviewer: Silvius Rus <srus@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <todd@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message