wangsheng 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 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/16606/5/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java: http://gerrit.cloudera.org:8080/#/c/16606/5/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java@87 PS5, Line 87: TableIdentifier oldTableId = IcebergUtil.getIcebergTableIdentifier(feTable); : try { > I think we should catch the UnsupportedException being thrown by Iceberg, a Done 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@3388 PS4, Line 3388: .ICEBERG > It's actually the 'new' metastore table. We have deep copied it to safely m In fact, I don't quite understand where the problem is. I write this method according to renameManagedKuduTable. It seem that both these two methods only update 'oldMsTbl' properties. Do you mean we cannot transfom new 'oldMsTbl' to HMS by getHiveClient().alter_table()? Besides, we cannot rename managed table of HadoopTables and HadoopCatalog, so I cannot test this situation yet. 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: * FLOAT -> DOUBLE : * DECIMAL(s1,p1) -> DECIMAL(s1,p2), same scale, p1<=p2 : */ > I don't think we need to re-implement the checks, we just need to provide g Ok, I would consider this when supporting alter column in a later patch! -- 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: 6 Gerrit-Owner: wangsheng <sky...@163.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: wangsheng <sky...@163.com> Gerrit-Comment-Date: Thu, 22 Oct 2020 14:42:10 +0000 Gerrit-HasComments: Yes
