Daniel Becker 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 7: (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: show_pattern:showPat > nit: this fits into the line above similarly to L2933 Done 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 It's a parser error, done. 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 that even if we have privileges on a different table in the same db, we : // still can't access 'iceberg_query_metadata' if we don't > I admit I'm not really familiar with these tests but I don't get this part Added a comment to clarify. 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 an Yes, we'll have to update this test if new metadata tables are added to Iceberg. 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 tabl Done 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 It really shouldn't exist but '_test_ranger_show_stmts_helper()' also deletes it first, I copied that approach. 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 Interestingly if I delete the database but do not drop the privileges next time I create the same db the privileges can still be there. This should not be the case here because I drop them in the finally block. Together with L123 I thought of this as an extra bit of cautiousness/robustness for the tests, but we can remove it. 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? The error message contains the username (which depends on where the test is run), so we would need to use for example regexes to match the errors. For me the error is: ``` AuthorizationException: User 'danielbecker' does not have privileges to access: test_ranger_show_iceberg_metadata_tables_1c0a89e8_db.ice ``` -- 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: 7 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 22 Feb 2024 10:47:12 +0000 Gerrit-HasComments: Yes
