Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling ...................................................................... Patch Set 2: (6 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java: http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java@29 PS2, Line 29: contiains > contains Done http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java@50 PS2, Line 50: Loads a native Iceberg table based on 'tableId' or 'tableLocation'. > I'm not sure if it is better to add: "'tableId' is for HadoopCatalog, 'tab Done http://gerrit.cloudera.org:8080/#/c/16575/2/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/16575/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1907 PS2, Line 1907: IcebergCatalogOpExecutor.dropTable((IcebergTable)existingTbl, params.if_exists); > Dropping synchronised tables isn't tightly related to the Iceberg catalog r For the current filesystem-based catalogs (HadoopTables and HadoopCatalog) the current behavior, i.e. let HMS delete the table directories work fine. But it won't work well with HiveCatalog I think. So I've added dropTable() functionality to the IcebergCatalog interface in this patch, and it seemed reasonable to also do this code change. With it, adding more catalogs should be really simple. But I can split this change to two if you feel strong about it. http://gerrit.cloudera.org:8080/#/c/16575/2/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/16575/2/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@80 PS2, Line 80: IcebergTable.isSynchronizedTable(feTable.getMetaStoreTable()) > We have already checked this condition in CatalogOpExecutor.java:1905, why This precondition ensures that this function cannot be called under the wrong circumstances. You're right, currently we also have this check in CatalogOpExecutor, but maybe that code will change in the future, or we'll have other places in the codebase that'll call this function. So it's basically a future-proof check. http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java File fe/src/main/java/org/apache/impala/util/IcebergUtil.java: http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@51 PS2, Line 51: import org.apache.impala.common.FileSystemUtil; > It seems that this import is unused Done http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@86 PS2, Line 86: loadTable > I see that these two 'loadTable' method only called in IcebergUtil class, m I will need this function from IcebergCatalogOpExecutor in my INSERT INTO patch (it currently uses IcebergUtil.getBaseTable()). I'm making the other one private, though probably these stateless utility functions don't cause much harm if they are public as they only invoke public methods. -- To view, visit http://gerrit.cloudera.org:8080/16575 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85 Gerrit-Change-Number: 16575 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[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: Mon, 12 Oct 2020 13:14:25 +0000 Gerrit-HasComments: Yes
