wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/16721 )
Change subject: IMPALA-10152: Add support for Iceberg HiveCatalog ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/16721/4/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/16721/4/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@419 PS4, Line 419: metadata_location This table property is generated by HiveCatalog, I'm curious about: 1. Can we set this table property when creating table? If not, I think we need to add some check in code; 2. Can we alter table to set this table property? If not, we also need to add some check in code; 3. Maybe we should define a static variable in IcebergTable.java, just like ICEBERG_CATALOG, and we can use a reference here. As far as I know, 'metadata_location' is refer to a metadata file's absolute path, and maybe we cannot modify this property manually. http://gerrit.cloudera.org:8080/#/c/16721/4/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHiveCatalog.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHiveCatalog.java: http://gerrit.cloudera.org:8080/#/c/16721/4/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHiveCatalog.java@65 PS4, Line 65: return hiveCatalog_.createTable(identifier, schema, spec, location, : properties); nits: One line is ok, unnecessary for two lines. http://gerrit.cloudera.org:8080/#/c/16721/4/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/16721/4/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@242 PS4, Line 242: * Get TIcebergFileFormat from a string, usually from table properties : */ Maybe we need add a comment here, since we change the default value to 'PARQUET' to replace 'null' -- To view, visit http://gerrit.cloudera.org:8080/16721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie574589a1751aaa9ccbd34a89c6819714d103197 Gerrit-Change-Number: 16721 Gerrit-PatchSet: 4 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: Fri, 20 Nov 2020 07:59:49 +0000 Gerrit-HasComments: Yes
