impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
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:

(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 <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message