Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20742 )
Change subject: IMPALA-11501: Add flag to allow catalog-cache operations on masked tables ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/20742/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20742/1//COMMIT_MSG@39 PS1, Line 39: always nit: not always, as getTableIfCached() will return the correct table getTable() was called first as the loaded table will be put to tables_. http://gerrit.cloudera.org:8080/#/c/20742/1/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java File fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java: http://gerrit.cloudera.org:8080/#/c/20742/1/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@127 PS1, Line 127: tbl = LocalTable.load(this, tblMeta); This looks strange to me, as the function's name is getTableIfCached, but we seem to load the table if it is not cached. I checked the Kudu path, and this will actually use the Kudu client to fetch metadata from Kudu, so not all metadata used here comes from the cache. This may be the right thing to do in local catalog, but some comments could be added to make this clearer. An alternative could be to call setMetaStoreTable(msTbl) on the LocalIncompleteTable so that the owner will be known. http://gerrit.cloudera.org:8080/#/c/20742/1/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@144 PS1, Line 144: FeTable tbl = getTableIfCached(tblName); : if (tbl instanceof LocalIncompleteTable) { : // The table exists but hasn't been loaded yet. : try { : tbl = LocalTable.load(this, tblName); This will load the table twice if loading it fails for some reason. Not really a bug but it looks strange. http://gerrit.cloudera.org:8080/#/c/20742/1/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20742/1/tests/authorization/test_ranger.py@1116 PS1, Line 1116: # getTableIfCached() in LocalCatalog loads a minimal incomplete table : # that does not include the ownership information. Hence show tables : # *never* show owned tables. TODO(bharathv): Fix in a follow up commit : pytest.xfail("getTableIfCached() faulty behavior, known issue") Doesn't your change fix this too? http://gerrit.cloudera.org:8080/#/c/20742/1/tests/authorization/test_ranger.py@1562 PS1, Line 1562: catalogd_args=LOCAL_CATALOG_CATALOGD_ARGS) : def test_allow_metadata_update_local_catalog(self, unique_name): Wouldn't it be better to run all tests with local catalog and add special handling to the ones where this makes a difference? It looks strange to me that several tests do not run with local catalog while it local catalog seems to be the main direction for Impala. -- To view, visit http://gerrit.cloudera.org:8080/20742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45935654cbf05a55d740f1b04781022c271f7678 Gerrit-Change-Number: 20742 Gerrit-PatchSet: 1 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 01 Dec 2023 12:10:41 +0000 Gerrit-HasComments: Yes
