Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
......................................................................


Patch Set 1:

(9 comments)

... a few more things

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

PS1, Line 223: Preconditions.checkNotNull(kuduDistributeByParams);
This breaks for tables that were created before this change.


http://gerrit.cloudera.org:8080/#/c/4414/1/fe/src/main/java/com/cloudera/impala/catalog/KuduTable.java
File fe/src/main/java/com/cloudera/impala/catalog/KuduTable.java:

PS1, Line 87: 
            :   // Key to specify the number of tablet replicas.
            :   // TODO(KUDU): Allow modification in alter table.
            :   public static final String KEY_TABLET_REPLICAS = 
"kudu.num_tablet_replicas";
I don't think we need to store this, even if we wanted to modify this later. 
Further, it doesn't look like the API exposes a way to do that anyway.

Can you add a TODO or remove this?


PS1, Line 92:   public static final String KEY_DISTRIBUTE_BY = 
"kudu.distribute_by";
I'm not sure we should be storing this. The Kudu client exposes 
KuduTable.getPartitionSchema(). It looks like it's just for use by SHOW CREATE 
TABLE, and it'd be good for us to use the PartitionSchema from them which we 
should probably be able to stringify.

I'm OK with leaving that for later as long as you think it won't be an issue 
for dev environments to upgrade between state now -> this review as-is -> 
making the change i suggested


http://gerrit.cloudera.org:8080/#/c/4414/1/testdata/workloads/functional-query/queries/QueryTest/create_kudu.test
File testdata/workloads/functional-query/queries/QueryTest/create_kudu.test:

we might still need to have explicit create table tests.

Specifically we should test that you can create an internal table with a 
different kudu table name by overriding 'kudu.table_name'. We could basically 
modify the managed_kudu table test case that's here to do so.

I think we need to find a way to modify the kudu schema (e.g. drop and add 
columns) and see if we handle gracefully. Seems OK to leave that as a future 
task, but we should at least know how we handle those cases.


http://gerrit.cloudera.org:8080/#/c/4414/1/testdata/workloads/functional-query/queries/QueryTest/kudu-show-create.test
File 
testdata/workloads/functional-query/queries/QueryTest/kudu-show-create.test:

well, we should have some coverage of show create table...


http://gerrit.cloudera.org:8080/#/c/4414/1/testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test
File 
testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test:

PS1, Line 15: 
            : 
            : 
            : 
            : 
            : 
            : 
            : 
            : 
            : 
            : 
I think this kind of error should still be covered. You can still mismatch PK 
cols and dist by columns.


PS1, Line 37: 
            : 
we should still cover this as well.


PS1, Line 50: 
            : 
same


Some more questions about losing coverage. Fine if this is now covered by FE 
tests.


-- 
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: 1
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: Matthew Jacobs <m...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to