Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
Patch Set 1:
... a few more things
PS1, Line 223: Preconditions.checkNotNull(kuduDistributeByParams);
This breaks for tables that were created before this change.
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.
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 =
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
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.
well, we should have some coverage of show create table...
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:
Some more questions about losing coverage. Fine if this is now covered by FE
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>