Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16606 )
Change subject: IMPALA-10166 (part 1): ALTER TABLE for Iceberg tables ...................................................................... Patch Set 4: (13 comments) Did a first round. The change looks good to me overall, but found a couple of nits. http://gerrit.cloudera.org:8080/#/c/16606/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16606/4//COMMIT_MSG@10 PS4, Line 10: supported nit: support http://gerrit.cloudera.org:8080/#/c/16606/4//COMMIT_MSG@15 PS4, Line 15: forbidden nit: forbid http://gerrit.cloudera.org:8080/#/c/16606/4//COMMIT_MSG@16 PS4, Line 16: caused nit: make http://gerrit.cloudera.org:8080/#/c/16606/4//COMMIT_MSG@17 PS4, Line 17: resolved column by field id column resolution by field id http://gerrit.cloudera.org:8080/#/c/16606/4//COMMIT_MSG@23 PS4, Line 23: . So we only rename the table in the Hive Metastore. http://gerrit.cloudera.org:8080/#/c/16606/4//COMMIT_MSG@24 PS4, Line 24: since Iceberg no related api since there is no API for that in Iceberg http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java: http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java@185 PS4, Line 185: not supported complex type on Iceberg tables. is not supported for complex types in Iceberg tables. http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java: http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java@93 PS4, Line 93: // We cannot replace column if exists, from primitive type to complex type or : // from complex type to primitive type I thought we don't want to support REPLACE COLUMNS in this patch at all. http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java: http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@98 PS4, Line 98: if (getTargetTable() instanceof FeKuduTable) analyzeKuduTable(analyzer); : else if (getTargetTable() instanceof FeIcebergTable) analyzeIcebergTable(analyzer); The coding convention is to use braces for multi-line statements, i.e. turn this into: if (...) { ... } else { ... } http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java: http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@100 PS4, Line 100: Cannot rename Hadoop tables Cannot rename Iceberg tables that use 'hadoop.tables' as catalog. http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@960 PS4, Line 960: // nit: // slashes should be indented http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3388 PS4, Line 3388: oldMsTbl It's actually the 'new' metastore table. We have deep copied it to safely modify it, then we'll use it in getHiveClient().alter_table() to update the table metadata in HMS. Seems like we just call it 'msTbl' at similar places in this file. http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/16606/4/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@112 PS4, Line 112: * INTEGER -> LONG : * FLOAT -> DOUBLE : * DECIMAL(s1,p1) -> DECIMAL(s1,p2), same scale, p1<=p2 Do we have checks for these? What does Iceberg do in cases when we violate these rules? We can deal with it in a later patch since alter column will be supported later. -- To view, visit http://gerrit.cloudera.org:8080/16606 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5104cc47c7b42dacdb52983f503cd263135d6bfc Gerrit-Change-Number: 16606 Gerrit-PatchSet: 4 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Wed, 21 Oct 2020 16:00:51 +0000 Gerrit-HasComments: Yes
