Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables ......................................................................
Patch Set 1: (3 comments) sorry, more comments I keep finding new things as I'm adding some new test cases myself. 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 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 metadata. Also: 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. http://gerrit.cloudera.org:8080/#/c/4414/1/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: PS1, Line 28: import com.cloudera.impala.catalog.KuduTable; : import junit.framework.Assert; nit: organize imports 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. 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 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
