Alex Behm has posted comments on this change. Change subject: IMPALA-2890: Support ALTER TABLE statements for Kudu tables ......................................................................
Patch Set 2: (18 comments) http://gerrit.cloudera.org:8080/#/c/5136/2/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: Line 185: 4: optional string location restore original numbers http://gerrit.cloudera.org:8080/#/c/5136/2/fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java: 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 that http://gerrit.cloudera.org:8080/#/c/5136/2/fe/src/main/java/org/apache/impala/analysis/AlterTableAddRangePartitionStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddRangePartitionStmt.java: Line 50: if (ifNotExists_) { single line Line 79: return; remove http://gerrit.cloudera.org:8080/#/c/5136/2/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java: Line 123: if (c.hasEncoding() || c.hasCompression() || c.hasBlockSize()) { Should we let Kudu handle this case and the one below? Does Kudu behave sanely? http://gerrit.cloudera.org:8080/#/c/5136/2/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java: Line 79: throw new AnalysisException("Key-value partition specs are not supported for " + ALTER TABLE DROP PARTITION is not supported for Kudu tables http://gerrit.cloudera.org:8080/#/c/5136/2/fe/src/main/java/org/apache/impala/analysis/AlterTableDropRangePartitionStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableDropRangePartitionStmt.java: Line 32: public class AlterTableDropRangePartitionStmt extends AlterTableStmt { code seems very similar to AlterTableAddRangePartitionStmt, should we just merge them into AlterTableAddDropRangePartitionStmt? Line 77: return; remove http://gerrit.cloudera.org:8080/#/c/5136/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: 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 UPDATE_STATS or SET_TBL_PROPERTIES 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. http://gerrit.cloudera.org:8080/#/c/5136/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: 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) http://gerrit.cloudera.org:8080/#/c/5136/2/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test 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 http://gerrit.cloudera.org:8080/#/c/5136/2/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: Line 99: assert kudu_client.table_exists(new_kudu_tbl_name) also check that the old one is gone -- To view, visit http://gerrit.cloudera.org:8080/5136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I04bc87e04e05da5cc03edec79d13cedfd2012896 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
