Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21026 )
Change subject: IMPALA-12609: Implement SHOW METADATA TABLES IN statement to list Iceberg Metadata tables ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/21026/6/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/21026/6/fe/src/main/cup/sql-parser.cup@2936 PS6, Line 2936: ident_or_default:tbl nit: this fits into the line above similarly to L2933 http://gerrit.cloudera.org:8080/#/c/21026/6/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/21026/6/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@2044 PS6, Line 2044: ParserError("SHOW TABLES IN db.tbl"); Can you add a test here where you don't provide a full path just the table name for SHOW METADATA TABLES? Or if it'd be an Analysis error could you add it to AnalyzeDDLTest? http://gerrit.cloudera.org:8080/#/c/21026/6/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/21026/6/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@1261 PS6, Line 1261: test.error(accessError("functional_parquet.iceberg_query_metadata"), : onTable("functional_parquet", "alltypes", privilege)); I admit I'm not really familiar with these tests but I don't get this part where we expect access error on 'iceberg_query_metadat' but we also have this onTable on 'alltypes.' http://gerrit.cloudera.org:8080/#/c/21026/6/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/21026/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@857 PS6, Line 857: show metadata tables in functional_parquet.iceberg_query_metadata; Just a note, that this test could break when we bump the Iceberg version and the new version has some new metadata tables. Not much we can do about this. http://gerrit.cloudera.org:8080/#/c/21026/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@879 PS6, Line 879: show metadata tables in functional_parquet.alltypestiny; Could you try something similar on a view that is on top of an Iceberg table? http://gerrit.cloudera.org:8080/#/c/21026/6/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/21026/6/tests/authorization/test_authorization.py@123 PS6, Line 123: admin_client.execute("drop database if exists {} cascade".format(unique_db), : user=ADMIN) : admin_client.execute("create database {}".format(unique_db), user=ADMIN) I'm wondering what is the case when this db exists already. We drop this in the finally section so it couldn't be remaining from previous failed tests. http://gerrit.cloudera.org:8080/#/c/21026/6/tests/authorization/test_authorization.py@129 PS6, Line 129: # Make sure there is no privilege stuck from the previous execution of the test. : admin_client.execute("revoke {priv} on database {db} from user {user}".format( : priv=priv, db=unique_db, user=user), user=ADMIN) Similarly to above, there can't be any remaining privs from previous tests since you remove priv in the finally block. http://gerrit.cloudera.org:8080/#/c/21026/6/tests/authorization/test_authorization.py@137 PS6, Line 137: assert str(exc1).strip() + non_existent_suffix == str(exc2).strip() Can't you assert on the actual error string? -- To view, visit http://gerrit.cloudera.org:8080/21026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide10ccf10fc0abf5c270119ba7092c67e712ec49 Gerrit-Change-Number: 21026 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[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, 22 Feb 2024 08:28:05 +0000 Gerrit-HasComments: Yes
