Daniel Becker 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: (8 comments) Thanks Tamas! 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 sounds more like "we know it's a metadata table and we analyse it" than "let's check whether it's a metadata table". Maybe 'check[If]IcebergMetadataTable' would be better. 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". 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 for non-metadata tables. 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 table? Shouldn't it be indicated? 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. 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 guarantees the value is null if it is not set. 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? 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 '*files' tables, such as 'data_files' and 'delete_files' etc.? -- 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 <tma...@apache.org> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Fri, 10 Nov 2023 13:03:12 +0000 Gerrit-HasComments: Yes
