impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
Date Mon, 17 Oct 2016 23:55:45 GMT
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 <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message