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

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