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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Martin Grund <[email protected]> Gerrit-Reviewer: Silvius Rus <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
