Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/20695 )
Change subject: IMPALA-12495: Describe statement for Iceberg metadata tables ...................................................................... Patch Set 2: (9 comments) Thank you for the review Daniel! Updated the patch. http://gerrit.cloudera.org:8080/#/c/20695/1/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/20695/1/be/src/service/frontend.cc@183 PS1, Line 183: if (params.__isset.metadata_table_name) tparams.__set_metadata_table_name( > line too long (104 > 90) Done http://gerrit.cloudera.org:8080/#/c/20695/2/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java File fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java: http://gerrit.cloudera.org:8080/#/c/20695/2/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java@96 PS2, Line 96: analyzeIcebergMetadataTable > I found the name 'analyzeIcebergMetadataTable' a bit misleading, to me it s Indeed, I decided to move the 'isIcebergMetadataTable' outside of the 'analyzeIcebergMetadataTable' method. http://gerrit.cloudera.org:8080/#/c/20695/2/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java File fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java: http://gerrit.cloudera.org:8080/#/c/20695/2/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java@358 PS2, Line 358: a > Nit: "an". Done http://gerrit.cloudera.org:8080/#/c/20695/2/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/20695/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1642 PS2, Line 1642: * Returns table metadata, such as the column descriptors, in the specified table. > Could mention 'vTableName' in the comment, including that it should be null Refactored this part, let me know your thoughts. http://gerrit.cloudera.org:8080/#/c/20695/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1650 PS2, Line 1650: String.format("fetching table %s.%s", tableName.db_name, tableName.table_name)); > Isn't this message misleading if we are actually looking up a metadata tabl The "parent" table is being loaded for this query. The metadata table does not exist in the catalog. http://gerrit.cloudera.org:8080/#/c/20695/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1708 PS2, Line 1708: Preconditions.checkArgument(outputStyle == TDescribeOutputStyle.FORMATTED || > Optional: could add a Precondition check that 'vTableName' is null. Done http://gerrit.cloudera.org:8080/#/c/20695/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/20695/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@503 PS2, Line 503: metadata_table_name > Shouldn't we check whether this param is set? I don't know if Thrift guaran Taken care of as part of the refactoring. http://gerrit.cloudera.org:8080/#/c/20695/2/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java: http://gerrit.cloudera.org:8080/#/c/20695/2/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java@273 PS2, Line 273: "" > Shouldn't it be null instead? With the refactoring this changed. http://gerrit.cloudera.org:8080/#/c/20695/2/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test: http://gerrit.cloudera.org:8080/#/c/20695/2/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@618 PS2, Line 618: describe functional_parquet.iceberg_query_metadata.all_data_files; > Shouldn't the 'all_*_files' tables be grouped together with the other '*fil You mean to not duplicate them? The reason I kept them separate is that there is an Iceberg library class representation for each of these, such as https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/AllDataFilesTable.java -- To view, visit http://gerrit.cloudera.org:8080/20695 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe22f271a59a6885035991c09b5101193ade6e97 Gerrit-Change-Number: 20695 Gerrit-PatchSet: 2 Gerrit-Owner: Tamas Mate <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Comment-Date: Tue, 14 Nov 2023 11:28:25 +0000 Gerrit-HasComments: Yes
