Matthew Jacobs has posted comments on this change.

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

Patch Set 1:


sorry, more comments

I keep finding new things as I'm adding some new test cases myself.
File fe/src/main/java/com/cloudera/impala/catalog/

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 = 
> I don't think we need to store this, even if we wanted to modify this later
After looking a bit more, I see that this is the way the user specifies the # 
of replicas and there is no other way. I guess it's weird to strip out table 
properties added by the user, so I suppose we should keep it. Instead, it'd be 
worth us considering separating out options that need to be persisted (i.e. we 
need the mapping to kudu tbl name) vs things that just get passed through and 
we shouldn't get in the business of trying to replicate in our own metadata. 
Probably not P1 but worth a JIRA since this is user facing and will sit in 

1) the TODO(KUDU) that is here could be removed.
2) would be worth testing how this behaves with CREATE TABLE LIKE. I'd expect 
it to inherit this value but it'd be worth checking.
File fe/src/test/java/com/cloudera/impala/analysis/

PS1, Line 28: import com.cloudera.impala.catalog.KuduTable;
            : import junit.framework.Assert;
nit: organize imports
File testdata/workloads/functional-query/queries/QueryTest/create_kudu.test:

> we might still need to have explicit create table tests.
Also, I don't see any tests for CTAS here or anywhere.

Can you add a few positive (e.g. from kudu_functional.alltypes) and negative 
tests (e.g. involving datatypes that kudu doesn't support, e.g. 
functional.alltypes and functional.decimaltbl ).

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

Reply via email to