Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-2890: Support ALTER TABLE statements for Kudu tables ......................................................................
Patch Set 2: (19 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 Oops, sorry about that. Done 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 Done 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 Done Line 79: return; > remove Done 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 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. http://gerrit.cloudera.org:8080/#/c/5136/2/fe/src/main/java/org/apache/impala/analysis/AlterTableChangeColStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableChangeColStmt.java: 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. 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 Done 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 Done Line 77: return; > remove Done 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 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 Done 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" 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)? Done 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. 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. Done 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 values. 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 Done 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 Done 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 Done -- 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
