impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2890: Support ALTER TABLE statements for Kudu tables
Date Tue, 22 Nov 2016 06:51:42 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-2890: Support ALTER TABLE statements for Kudu tables

Patch Set 2:

File common/thrift/JniCatalog.thrift:

Line 185:   4: optional string location
> restore original numbers
Oops, sorry about that. Done
File fe/src/main/java/org/apache/impala/analysis/

Line 88:       throw new AnalysisException("Key-value partition specs are not supported for
" +
> how about "ALTER TABLE ADD PARTITION is not supported for Kudu tables." or 
File fe/src/main/java/org/apache/impala/analysis/

Line 50:     if (ifNotExists_) {
> single line

Line 79:     return;
> remove
File fe/src/main/java/org/apache/impala/analysis/

Line 123:         if (c.hasEncoding() || c.hasCompression() || c.hasBlockSize()) {
> Should we let Kudu handle this case and the one below? Does Kudu behave san
Unfortunately we can't do that in this case. There is no Kudu API call that accepts all these
options, so we have to reject them here.
File fe/src/main/java/org/apache/impala/analysis/

Line 103:     if (newColDef_.hasKuduOptions()) {
> we cannot change the default value?
Currently not supported. They are working on changing this behavior but it's not there yet.
File fe/src/main/java/org/apache/impala/analysis/

Line 79:       throw new AnalysisException("Key-value partition specs are not supported for
" +
> ALTER TABLE DROP PARTITION is not supported for Kudu tables
File fe/src/main/java/org/apache/impala/analysis/

Line 32: public class AlterTableDropRangePartitionStmt extends AlterTableStmt {
> code seems very similar to AlterTableAddRangePartitionStmt, should we just 

Line 77:     return;
> remove
File fe/src/main/java/org/apache/impala/service/

Line 553:       case UPDATE_STATS:
> I think we should be able to avoid the copy+paste by adding an additional c
Good idea. Done

Line 2231:               KuduCatalogOpExecutor.renameTable((KuduTable) tbl,
> I don't think we should rename the Kudu table for external tables. An exter
File fe/src/test/java/org/apache/impala/analysis/

Line 1831:     String[] addDrop = {"add if not exists", "drop if exists"};
> what about just "add" and "drop"
Added. Done

Line 1868:     AnalysisError("alter table functional_kudu.testtbl add columns (a1 int)",
> How do we know a1 is going to be non-nullable at this point (in analysis)? 

Line 1875:     AnalysisError("alter table functional_kudu.testtbl add columns " +
> use a loop to cover the cases separately (if you decide to keep these check
As I mentioned in a previous comment, we can't defer these to Kudu (not supported API), so
they have to be checked during the analysis. Added the tests here.
File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test:

Line 52:   distribute by range (id) (partition 1 < values <= 10) stored as kudu
> Test dropping all partitions and scanning/inserting into the empty table.

Line 156: # Insert a row that doesn't have values for the new columns; defaults are used
> Sorry this doesn't make sense. I was thinking of ALTERing the default value
No worries, we will have to add tests like that when we have the ability to change default

Line 157: insert into tbl_to_alter (id,name,vali) values (3, 'test', 200)
> Can you try this same test but with NULLs for the new columns? The insert s

Line 257: create table copy_of_tbl (a int primary key) distribute by hash (a) into 3 buckets
> new test for the external table "pointer" behavior
File tests/query_test/

Line 99:     assert kudu_client.table_exists(new_kudu_tbl_name)
> also check that the old one is gone

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I04bc87e04e05da5cc03edec79d13cedfd2012896
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-HasComments: Yes

View raw message