impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2890: Support ALTER TABLE statements for Kudu tables
Date Mon, 21 Nov 2016 23:54:15 GMT
Alex Behm 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
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 something like
File fe/src/main/java/org/apache/impala/analysis/

Line 50:     if (ifNotExists_) {
single line

Line 79:     return;
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 sanely?
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 merge them into

Line 77:     return;
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 check to L365 that
will make us only go down this patch for ADD_REPLACE_COLUMNS, DROP_COLUMN, etc., but not for

Maybe something like altersKuduTable(TAlterTableType type), you get the idea

Line 2231:               KuduCatalogOpExecutor.renameTable((KuduTable) tbl,
I don't think we should rename the Kudu table for external tables. An external table is like
a pointer, and changing the tblprpperty should just point it to another table imo.
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"

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)? That's up to Kudu's
default right?

Probably best to add an explicit NOT NULL in any case.

Line 1875:     AnalysisError("alter table functional_kudu.testtbl add columns " +
use a loop to cover the cases separately (if you decide to keep these checks part of analysis
and not defer to Kudu)
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
New test somewhere:
* drop new_col2
* re-add new_col2 but with a different default value
* insert a new row, triggering the new default value
* check that old rows have the old default value and new rows have the new default value

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 should give errors.

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