impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2890: Support ALTER TABLE statements for Kudu tables
Date Mon, 21 Nov 2016 21:05:37 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-2890: Support ALTER TABLE statements for Kudu tables

Patch Set 1:

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
File fe/src/main/java/org/apache/impala/analysis/

Line 56:   public AlterTableAddPartitionStmt(TableName tableName,
> Seems to make more sense to move this into a new AlterTableAddRangePartitio
File fe/src/main/java/org/apache/impala/analysis/

Line 85:       throw new AnalysisException("ALTER TABLE REPLACE COLUMNS not currently " +
> is not supported on Kudu tables.
File fe/src/main/java/org/apache/impala/analysis/

Line 109:       if (col.getType() != newColDef_.getType()) {
> use equals()
File fe/src/main/java/org/apache/impala/analysis/

Line 52:   public AlterTableDropPartitionStmt(TableName tableName,
> Separate this into a new AlterTableDropRangePartitionStmt?
File fe/src/main/java/org/apache/impala/analysis/

Line 99:       throw new AnalysisException(String.format(
> Let's move this check into the respective alter statements that are not sup
File fe/src/main/java/org/apache/impala/analysis/

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,? 
File fe/src/main/java/org/apache/impala/analysis/

Line 232:         // We shouldn't output the columns for external tables
> good catch
File fe/src/main/java/org/apache/impala/catalog/

Line 174:     return ImmutableList.copyOf(rangeDistribution.getColumnNames());
> getColumnNames() already returns an ImmutableList
Thanks. Done

Line 267:         ColumnSchema col =
> add helper getColumnById()?

Line 268:           tableSchema.getColumnByIndex(tableSchema.getColumnIndex(colId));
> indent 4
File fe/src/main/java/org/apache/impala/service/

Line 366:           if (tbl instanceof KuduTable) {
> Consider adding a separate switch() only for Kudu tables above, or some oth

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

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.
File fe/src/main/java/org/apache/impala/service/

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

Line 337:    * Returns the bounds of a range partition in two PartialRow, RangePartitionBound
> 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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I04bc87e04e05da5cc03edec79d13cedfd2012896
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-HasComments: Yes

View raw message