impala-reviews mailing list archives

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

(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 <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