Marcel Kornacker 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 ALL KEYWORDS ALSO NEED TO BE ADDED TO THE ident_or_kw PRODUCTION 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? 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? 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" 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 Line 141: + "(Type: %s) is not supported.", expr.getStringValue(), expr.getType().toSql())); remove trailing 'is not supported'. 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? 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 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 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. 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? 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? 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 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. Line 126: cto.addHashPartitions(dP.getBy_hash_param().getColumns(), also checkstate that by_range_param isn't set 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 Line 178: cardinality_ = Math.max(Math.max(1, cardinality_), kuduTable_.getNumRows()); needs to be min(max(1, card), #rows). 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 by the table. Line 78: * Enum to specify the sink operation type. move above member vars. thanks for restructuring this, it looks much cleaner now, a definite improvement. 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 PartialRow. Line 163: private static void setKey(PartialRow key, Type type, TRangeLiteral literal, int pos, same here Line 168: key.addBoolean(pos, literal.bool_literal); use getter here as well for consistency 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 -- 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
