Alex Behm has posted comments on this change. Change subject: IMPALA-2890: Support ALTER TABLE statements for Kudu tables ......................................................................
Patch Set 1: (20 comments) First wave of comments. Will look at tests next. http://gerrit.cloudera.org:8080/#/c/5136/1/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: Line 179: 2: optional CatalogObjects.TRangePartition range_partition_spec Seems to make more sense to completely separate the HDFS partition case from the Kudu range-partition case. http://gerrit.cloudera.org:8080/#/c/5136/1/fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java: Line 56: public AlterTableAddPartitionStmt(TableName tableName, Seems to make more sense to move this into a new AlterTableAddRangePartitionStmt since this new case is totally different from the HDFS partition case. http://gerrit.cloudera.org:8080/#/c/5136/1/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java: Line 85: throw new AnalysisException("ALTER TABLE REPLACE COLUMNS not currently " + is not supported on Kudu tables. Line 142: throw new AnalysisException("Unsupported column options: " + c.toString()); would be nice to say why they are unsupported (only supported on Kudu tables) http://gerrit.cloudera.org:8080/#/c/5136/1/fe/src/main/java/org/apache/impala/analysis/AlterTableChangeColStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableChangeColStmt.java: Line 109: if (col.getType() != newColDef_.getType()) { use equals() http://gerrit.cloudera.org:8080/#/c/5136/1/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java: Line 52: public AlterTableDropPartitionStmt(TableName tableName, Separate this into a new AlterTableDropRangePartitionStmt? http://gerrit.cloudera.org:8080/#/c/5136/1/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java: Line 99: throw new AnalysisException(String.format( Let's move this check into the respective alter statements that are not supported on Kudu tables. Pretty long list of allowed statements we have here. http://gerrit.cloudera.org:8080/#/c/5136/1/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java: Line 162: public boolean hasEncoding() { return encoding_ != null; } use these in hasKuduOptions() Line 322: return new ColumnDef(col.getName(), new TypeDef(col.getType())); What about all the other column properties like comment/primary_key, etc,? I know you don't need the for your use case here. Might be simpler to move this code to the caller to avoid confusion about what the returned ColumnDef here actually has (and has not). http://gerrit.cloudera.org:8080/#/c/5136/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: Line 232: // We shouldn't output the columns for external tables good catch http://gerrit.cloudera.org:8080/#/c/5136/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: Line 174: return ImmutableList.copyOf(rangeDistribution.getColumnNames()); getColumnNames() already returns an ImmutableList Line 267: ColumnSchema col = add helper getColumnById()? Line 268: tableSchema.getColumnByIndex(tableSchema.getColumnIndex(colId)); indent 4 http://gerrit.cloudera.org:8080/#/c/5136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: Line 366: if (tbl instanceof KuduTable) { Consider adding a separate switch() only for Kudu tables above, or some other way of making the distinction more clear in the code. Interspersing calls to KuduCatalogOpExecutor() with a break seems a little strange. The Kudu operations really are totally separate so ideally the code structure would reflect that. Line 2168: if (properties.containsKey(KuduTable.KEY_TABLE_NAME) && This seems to only allow renaming the Kudu table if the old Kudu table already existed. I think a more common use case is actually that somebody got the initial Kudu table name wrong and now wants to change it to something valid. What do you think? Line 2172: KuduCatalogOpExecutor.renameTable((KuduTable) tbl, will this succeed if the old table doesn't exist? http://gerrit.cloudera.org:8080/#/c/5136/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: Line 300: if (!ifNotExists) { we really need finer grained error handling here :( I know it's currently not possible because we cannot distinguish the different Kudu errors, right? Line 337: * Returns the bounds of a range partition in two PartialRow, RangePartitionBound pairs enclose the pair members in <> Line 393: public static void dropColumn(KuduTable tbl, String colName) I assume we can't drop PK columns. Let's check for that in analysis. Line 409: public static void renameColumn(KuduTable tbl, String oldName, TColumn newCol) can PK columns be renamed? -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-HasComments: Yes
