Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19483 )
Change subject: IMPALA-11908: Parser change for Iceberg metadata querying ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/19483/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19483/4//COMMIT_MSG@13 PS4, Line 13: . Nit: comma? http://gerrit.cloudera.org:8080/#/c/19483/4//COMMIT_MSG@14 PS4, Line 14: metadat Nit: metadata. http://gerrit.cloudera.org:8080/#/c/19483/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/19483/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3289 PS4, Line 3289: database 'dbName' is no longer a parameter of this function. http://gerrit.cloudera.org:8080/#/c/19483/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3337 PS4, Line 3337: * Adds a new metadata table to the stmt table cache. At this point it is unknown if Could add that this is for Iceberg only (at least now). http://gerrit.cloudera.org:8080/#/c/19483/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3342 PS4, Line 3342: MetaVirtual MetadataVirtualTable? http://gerrit.cloudera.org:8080/#/c/19483/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3349 PS4, Line 3349: virtualTableName It's a bit confusing that we get 'originalTable' from 'virtualTableName' and not for example 'originalTableName'. http://gerrit.cloudera.org:8080/#/c/19483/4/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/19483/4/fe/src/main/java/org/apache/impala/analysis/TableName.java@42 PS4, Line 42: vTbl_ Could add a comment describing what 'vTbl_' is used for. http://gerrit.cloudera.org:8080/#/c/19483/4/fe/src/main/java/org/apache/impala/analysis/TableName.java@55 PS4, Line 55: Preconditions.checkNotNull(db); In the other constructor on L44, we check that db == null || !db.isEmpty() I guess we don't allow 'db' to be null because of what is written on L82, but do we allow 'db.isEmpty()'? http://gerrit.cloudera.org:8080/#/c/19483/4/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/19483/4/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@36 PS4, Line 36: import org.apache.impala.catalog.HdfsPartition.FileDescriptor; : import org.apache.impala.common.Pair; Unused imports. http://gerrit.cloudera.org:8080/#/c/19483/4/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/19483/4/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java@95 PS4, Line 95: Boolean Can't we return a primitive 'boolean'? http://gerrit.cloudera.org:8080/#/c/19483/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/19483/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@2216 PS4, Line 2216: that are Nit: "which is currently not supported" would be better. -- To view, visit http://gerrit.cloudera.org:8080/19483 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0b5db884b5f3fecbd132fcb2c2cbd6c622ff965b Gerrit-Change-Number: 19483 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 09 Feb 2023 12:29:38 +0000 Gerrit-HasComments: Yes
