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.
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.
PS1, Line 28: import com.cloudera.impala.catalog.KuduTable;
: import junit.framework.Assert;
nit: organize imports
> 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-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>