Alex Behm has posted comments on this change.

Change subject: IMPALA-3650: DISTRIBUTE BY required for managed Kudu tables
......................................................................


Patch Set 2:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/3987/1/fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java:

Line 379:       List<String> keyColumns = KuduUtil.parseKeyColumnsAsList(
so DISTRIBUTE BY is allowed and just ignored for external tables?

seems like cleaning that up fits well with the other external table 
simplifactions, so maybe add a TODO for now


Line 388:           "A data distribution must be specified using a DISTRIBUTE 
BY clause.");
Data distribution must be specified using the DISTRIBUTE BY clause.


http://gerrit.cloudera.org:8080/#/c/3987/2/fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java:

Line 378:     if (distributeParams_ != null) {
I guess we just ignore the DISTRIBUTE BY clause for external tables? Seems like 
we would want to throw an error.


Line 388:           "A data distribution must be specified using a DISTRIBUTE 
BY clause.");
the DISTRIBUTE BY clause


http://gerrit.cloudera.org:8080/#/c/3987/2/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java:

Line 1675:         "distribute by hash into 2 buckets tblproperties (" +
remind me: do we distribute by the primary key if no columns are given?


Line 1773:     // DISTRIBUTE BY is not required for external tables.
must not be specified for external tables?


http://gerrit.cloudera.org:8080/#/c/3987/2/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:

Line 133:   DISTRIBUTE BY HASH (l_orderkey) INTO 3 BUCKETS
is this distribution across tablet servers or tablets? just asking because 3 
buckets might seem low depending on the answer


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb15110b10b28ef6dd8ec136c2522b5f44dca43e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <[email protected]>
Gerrit-Reviewer: Alex Behm <[email protected]>
Gerrit-HasComments: Yes

Reply via email to