Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/19547/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19547/1//COMMIT_MSG@9 PS1, Line 9: , this Nit: should start a new sentence here. http://gerrit.cloudera.org:8080/#/c/19547/1//COMMIT_MSG@11 PS1, Line 11: writing the backend : part Is it going to be in another patch or this one? If another one, can you indicate that? http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/analysis/TableName.java File fe/src/main/java/org/apache/impala/analysis/TableName.java: http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/analysis/TableName.java@59 PS1, Line 59: Preconditions.checkNotNull(tbl); Is it allowed to be empty? http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java: http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java@52 PS1, Line 52: metadataTableName_ = tblRefPath.get(2); Can there be interference with for example CollectionTableRefs which may have a path longer than 2? Or is this function only called if we know this is not the case? http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java File fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java: http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java@36 PS1, Line 36: desc_ = desc; Do we allow a null desc_? If not, we could have a precondition check. http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java@47 PS1, Line 47: Preconditions.checkNotNull(desc_.getPath()); If desc_ can be null (see comment on L36), we should also check that it is not null here. http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/service/Frontend.java@2217 PS1, Line 2217: t Nit: extra t. http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/service/Frontend.java@2220 PS1, Line 2220: t Nit: extra t. Also, shouldn't it be "metadata table"? -- To view, visit http://gerrit.cloudera.org:8080/19547 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6 Gerrit-Change-Number: 19547 Gerrit-PatchSet: 1 Gerrit-Owner: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Anonymous Coward <lipeng...@apache.org> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <gfurnst...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 06 Mar 2023 14:49:19 +0000 Gerrit-HasComments: Yes
