Alex Behm has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables ......................................................................
Patch Set 7: (26 comments) Next round over the code. http://gerrit.cloudera.org:8080/#/c/4414/7/be/src/service/frontend.cc File be/src/service/frontend.cc: Line 62: "value should be a comma separated list of hostnames or IP addresses."); are ports optional or mandatory? http://gerrit.cloudera.org:8080/#/c/4414/7/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 358: 1: required list<TRangeLiteral> values Why not a list of TExpr that are expected to be literals? Seems more future proof. Line 368: // the type parameter. which type parameter? http://gerrit.cloudera.org:8080/#/c/4414/7/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: Line 398: 14: optional list<CatalogObjects.TDistributeParam> distribute_by; for consistency let's remove trailing ";" http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 1033: // class doesn't inherit from CreateTableStmt. whitespace Line 1047: // Used for creating external Kudu tables for which the schema is loaded from Kudu. There seem to be more uses of this production, so this comment could be misleading. Maybe generalize to something like "Used for creating tables where the schema is inferred externally, e.g., from an Avro schema, Kudu table or query statement." Line 1112: // or one RANGE clause typo: clauses http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java: Line 93: void setIsPrimaryKey() { isPrimaryKey_ = true; } do we need this? Line 191: Preconditions.checkState(!colDefsByColName.containsKey(colDef.getColName())); can check return value of put() http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: Line 208: * Analyzes and checks table properties which are common for both managed and external typo: common to Line 255: "PARTITIONED BY cannot be used with an Kudu table."); typo: a Kudu table this also needs to be checked for managed tables right? Line 273: AnalysisUtils.throwIfNullOrEmpty(getPrimaryKeyColumnDefs(), Shouldn't this check hasPrimaryKey()? Line 284: "zero. Given number of replicas is: " + r.toString() + ".'"); remove trailing .' or add the opening single-quote Line 318: private boolean hasPrimaryKey() { Isn't it enough to check primaryKeyColDefs_ in tableDef_? http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/DistributeParam.java File fe/src/main/java/org/apache/impala/analysis/DistributeParam.java: Line 105: for (String colName: colNames_) { we could specify the same distribution column multiple times Line 127: throw new AnalysisException("Split values cannot be NULL"); do we have a test for this? Line 223: colNames_.addAll(colNames); do we need toLower()? http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: Line 69: // Populated during analysis. Authoritative list of primary key column definitions populated during analysis. Line 177: fqTableName_.analyze(); Do you know if Kudu has more permissive or more restrictive constraints on what strings can be used as table/column names? I'd be surprised if HMS and Kudu were identical in that respect. Better to file a JIRA and leave that investigation/fix out of this patch. Line 181: if (analyzer.dbContainsTable(getTblName().getDb(), getTbl(), Privilege.CREATE) Are we going to check Sentry privs for Kudu tables? Also ok to defer this fix, but let's not forget. Line 220: * Analyzes the primary key columns. Primary keys are only supported for Kudu Replace the second sentence with a brief description what this checks. It does not check the format and succeeds if no primary keys are given, so there is nothing Kudu specific here (that is checked in CreateTableStmt). Line 234: StringBuilder columnDefStr = new StringBuilder(); Not used? http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: Line 1117: if (db != null && params.cascade) dropTablesFromKudu(db); I think it might be a good idea to do this under the metastoreDdlLock_ as well to ensure that the Kudu and HMS table deletions are atomic. http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: Line 47: /** newline before Line 231: } catch (Exception e) { Did you address my comment more nuanced checking here to distinguish connection issues from "table does not exist"? http://gerrit.cloudera.org:8080/#/c/4414/7/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: Line 204: CREATE TABLE {table} (c INT PRIMARY KEY) add comment to the PK column -- To view, visit http://gerrit.cloudera.org:8080/4414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-HasComments: Yes
