Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/19483 )
Change subject: IMPALA-11908: Parser change for Iceberg metadata querying ...................................................................... Patch Set 6: (8 comments) Thanks all for the review, updated the patch. http://gerrit.cloudera.org:8080/#/c/19483/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19483/5//COMMIT_MSG@12 PS5, Line 12: statemen > typo: statement Done http://gerrit.cloudera.org:8080/#/c/19483/5//COMMIT_MSG@22 PS5, Line 22: NotImplementedException. > typo: NotImplementedException Done 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@a1323 PS4, Line 1323: > Thanks for explain! Thanks for describing it further. Sorry, I thought you were asking about rawPath of this method. This subList can contain values when querying nested types, for example: describe functional_parquet.allcomplextypes.map_map_col; In this case the new Path object will have map_map_col as rawPath. http://gerrit.cloudera.org:8080/#/c/19483/5/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/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@876 PS5, Line 876: if (table instanceof IcebergMetadataTable) { : return new IcebergMetadataTableRef(tableRef, resolvedPath); > nit: multi-line if statements should use brackets Done 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@55 PS4, Line 55: public TableName(String db, String tbl, String vTbl) { > If none should be null or empty, shouldn't the condition be Indeed, this broke a few things, updated these conditions. http://gerrit.cloudera.org:8080/#/c/19483/5/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/5/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@417 PS5, Line 417: , > We could also pass 'e' as the second parameter so we'd get more context whe Done http://gerrit.cloudera.org:8080/#/c/19483/5/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/5/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java@39 PS5, Line 39: metadata > typo: metadata Done http://gerrit.cloudera.org:8080/#/c/19483/5/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/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@2215 PS5, Line 2215: metadtata > typo: metadata, and probably write "metadata table" Done -- 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: 6 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: Mon, 27 Feb 2023 13:19:37 +0000 Gerrit-HasComments: Yes
