Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19194 )
Change subject: IMPALA-10986: Require the SELECT privilege to execute a UDF ...................................................................... Patch Set 9: (5 comments) I slightly revised the patch according to Csaba's suggestions in the previous review. We probably have to wait for https://gerrit.cloudera.org/c/19252/ (IMPALA-11728) to be merged/resolved since that patch is also related to this one. http://gerrit.cloudera.org:8080/#/c/19194/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19194/7//COMMIT_MSG@16 PS7, Line 16: tables, columns in the database where the UDF belongs to. : > Can you add this info to the commit message? Thanks Csaba! I will revise the commit message in the next patch. IMPALA-11769 was also created to keep track of the difference in behavior between Impala and Hive. http://gerrit.cloudera.org:8080/#/c/19194/8/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java: http://gerrit.cloudera.org:8080/#/c/19194/8/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@345 PS8, Line 345: if (path.size() == 2 && I will change the conditions to the following in the next patch so that wildcard function names in the forms of `*`.`*`, <db_name>.`*`, or `*`.<fn_name> are supported. if (path.size() == 2 && (path.get(0).equals("*") || path.get(1).equals("*"))) { http://gerrit.cloudera.org:8080/#/c/19194/8/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@352 PS8, Line 352: > At this point db name cannot be *, right? Can you add a precondition about At this point 'dbName_' is still null since it is not set up until https://gerrit.cloudera.org/c/19194/8/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java#357. On the other hand, 'db_' and 'fn_' of 'functionName_' will be set up after functionName_.analyze(). In this regard, I will add the following after functionName_.analyze() in my next patch. Preconditions.checkArgument(!functionName_.getDb().equals("*")); Preconditions.checkArgument(!functionName_.getFunction().equals("*")); http://gerrit.cloudera.org:8080/#/c/19194/8/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/19194/8/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@53 PS8, Line 53: * Creates a new AuthorizationChecker based on the config values. > nit: unneeded change? Done http://gerrit.cloudera.org:8080/#/c/19194/8/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/19194/8/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@555 PS8, Line 555: ScalarFunction fn = addFunction("functional", "f"); > Can you also add another function, e.g. f2? A test could check the followin Thanks Csaba! I will add the following test cases in my next patch. .error(selectFunctionError("functional.f"), onDatabase("functional", TPrivilegeLevel.INSERT), onUdf("functional", "f2", TPrivilegeLevel.SELECT)) .error(selectFunctionError("functional.f"), onDatabase("functional", TPrivilegeLevel.REFRESH), onUdf("functional", "f2", TPrivilegeLevel.SELECT)) -- To view, visit http://gerrit.cloudera.org:8080/19194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5e58ba30545ce169786aac279b00c8f6e09ae740 Gerrit-Change-Number: 19194 Gerrit-PatchSet: 9 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Aman Sinha <[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: Thu, 01 Dec 2022 04:41:54 +0000 Gerrit-HasComments: Yes
