Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19252 )
Change subject: IMPALA-11728: Set fallback database for functions ...................................................................... Patch Set 17: (3 comments) Hi all, I just left some more comments since the JIRA seems to be more complicated than I expected. Sorry that I did not realize it in my previous review. Let me know if my feedback is reasonable. Thanks! http://gerrit.cloudera.org:8080/#/c/19252/15/fe/src/main/java/org/apache/impala/analysis/FunctionName.java File fe/src/main/java/org/apache/impala/analysis/FunctionName.java: http://gerrit.cloudera.org:8080/#/c/19252/15/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@128 PS15, Line 128: preferBuiltinsDb Is it possible that 'preferBuiltinsDb' is false but fallbackDbContainsFn(analyzer) is true? I just found that in the CREATE/DROP FUNCTION statements, 'preferBuiltinsDb' could be false. Do we need to consider those 2 statements as well in this JIRA? It seems that for the CREATE/DROP FUNCTION statements, the current patch always uses the database returned by "analyzer.getDefaultDb()". Is my understanding correct? http://gerrit.cloudera.org:8080/#/c/19252/15/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@145 PS15, Line 145: FeDb feDb = analyzer.getDb(dbName, false); To address Csaba's concern at https://gerrit.cloudera.org/c/19252/15/tests/authorization/test_ranger.py#1151, we could probably change this to the following if we only want to consider the case when a user tries to execute UDF's in a SELECT query. This way the user has to be granted any of the INSERT, REFRESH, SELECT privileges on the fallback database even though a) the database does not exist or b) the database exists but the function does not. FeDb feDb = analyzer.getDb(dbName, Privilege.VIEW_METADATA, /* throwIfDoesNotExist */ false); However, if we would like to additionally consider the CREATE/DROP FUNCTION statements, then different privileges have to be registered. http://gerrit.cloudera.org:8080/#/c/19252/15/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/19252/15/tests/authorization/test_ranger.py@1146 PS15, Line 1146: result = self.execute_query_expect_failure( : non_owner_client, "select identity(1)", query_options={ : 'FALLBACK_DB_FOR_FUNCTIONS': unique_database}, user=test_user) : err = "User '{0}' does not have privileges to access: {1}".format( : test_user, unique_database) : assert err in str(result) > It makes sense. But currently it throw an Exception "default.fn() unknown f Thanks Xiaoqing for adding the test case and Csaba for pointing out a subtle case when a requesting user who explicitly specifies a fallback database 'FALLBACK_DB_FOR_FUNCTIONS' that does not contain the function! I did not discover this in my previous review. I think it should be possible to not change the current precedence proposed by Xiaoqing. Specifically, in FunctionName#FALLBACK_DB_FOR_FUNCTIONS(), instead of calling "analyzer.getDb(dbName, /* throwIfDoesNotExist */ false)", we call "analyzer.getDb(dbName, Privilege.VIEW_METADATA, /* throwIfDoesNotExist */ false)". This way we also register a privilege request for the fallback database even though a) the database does not exist or b) the database exists but the function does not. As a result, Impala's frontend will still check with the Ranger plugin to see if the requesting user is granted any of the REFRESH, INSERT, SELECT privileges on the fallback database. This works if FunctionName#fallbackDbContainsFn() is only invoked in a SELECT query in that to execute a UDF of a database in a SELECT query, Impala currently requires the requesting user to be granted any of those 3 privileges on the database as mentioned above. However, if we need to consider the CREATE/DROP FUNCTION statements as well in this JIRA, we will have to register different privilege requests accordingly. Hence, depending on whether we would like to consider the CREATE/DROP FUNCTION statements in this JIRA, different test cases should be added. -- To view, visit http://gerrit.cloudera.org:8080/19252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I37b7e126718fea1c50723cacbaed898b20bb55e5 Gerrit-Change-Number: 19252 Gerrit-PatchSet: 17 Gerrit-Owner: Xiaoqing Gao <[email protected]> Gerrit-Reviewer: Anonymous Coward <[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-Reviewer: Xiaoqing Gao <[email protected]> Gerrit-Comment-Date: Wed, 30 Nov 2022 10:09:28 +0000 Gerrit-HasComments: Yes
