Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-2890: Support ALTER TABLE statements for Kudu tables ......................................................................
Patch Set 1: (19 comments) 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 fro Done 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 AlterTableAddRangePartitio Done 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. Done 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() Done 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? Done 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 sup Done 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() Done Line 322: return new ColumnDef(col.getName(), new TypeDef(col.getType())); > What about all the other column properties like comment/primary_key, etc,? Done 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 Done 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 Thanks. Done Line 267: ColumnSchema col = > add helper getColumnById()? Done Line 268: tableSchema.getColumnByIndex(tableSchema.getColumnIndex(colId)); > indent 4 Done 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 oth Done Line 2168: if (properties.containsKey(KuduTable.KEY_TABLE_NAME) && > This seems to only allow renaming the Kudu table if the old Kudu table alre The Kudu table name property is always set even if the user does explicitly specify it, so I believe the case you mention is handled correctly here. Let me know if I misunderstood the comment. Line 2172: KuduCatalogOpExecutor.renameTable((KuduTable) tbl, > will this succeed if the old table doesn't exist? It will fail but the tbl properties haven't been updated at that point, so the HMS entry will not change. 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 :( Yeah, I don't like that either. KuduException is too generic and doesn't tell us much in this case. Line 337: * Returns the bounds of a range partition in two PartialRow, RangePartitionBound pairs > enclose the pair members in <> Done 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. Shouldn't we let Kudu handle this check? Line 409: public static void renameColumn(KuduTable tbl, String oldName, TColumn newCol) > can PK columns be renamed? Same as above. Shouldn't we let Kudu handle this? -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
