Xiaoqing Gao has posted comments on this change. ( http://gerrit.cloudera.org:8080/19252 )
Change subject: IMPALA-11728 Set fallback database for functions ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/19252/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19252/4//COMMIT_MSG@7 PS4, Line 7: [IMPALA-11728] > we generally use the IMPALA-11728: ... format. Done http://gerrit.cloudera.org:8080/#/c/19252/4/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/4/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@126 PS4, Line 126: preferBuiltinsDb > I think that we should use fallback db only if preferBuiltinsDb is true - t I checked that preferBuiltinsDb is false only in CreateFunctionStmtBase.java and DropFunctionStmt.java. I modified the if statement. http://gerrit.cloudera.org:8080/#/c/19252/4/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@128 PS4, Line 128: } else if (fallbackDbContainsFn(analyzer)) { > I am not sure about the correct order here - checking the fallback db first I'm not sure which function has higher priority, the fallback db or the _impala_builtins? In the past, _impala_builtins have higher priority than analyzer.getDefaultDb(). Maybe we can stay the same? http://gerrit.cloudera.org:8080/#/c/19252/4/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@129 PS4, Line 129: analyzer.getQueryCtx() : .getClient_request() : .getQuery_options() : .getFallback_db_for_functions(); > Can you create a function like String Analyzer.getFallbackDbForFunctions(), Done http://gerrit.cloudera.org:8080/#/c/19252/4/tests/custom_cluster/test_fallback_db_for_functions.py File tests/custom_cluster/test_fallback_db_for_functions.py: http://gerrit.cloudera.org:8080/#/c/19252/4/tests/custom_cluster/test_fallback_db_for_functions.py@28 PS4, Line 28: CustomClusterTestSuite > Since there is a query option for fallback_db_for_functions, a simple (non Done http://gerrit.cloudera.org:8080/#/c/19252/4/tests/custom_cluster/test_fallback_db_for_functions.py@36 PS4, Line 36: fn > It would be nice to also test a function name that also exists as builtin f Done http://gerrit.cloudera.org:8080/#/c/19252/4/tests/custom_cluster/test_fallback_db_for_functions.py@39 PS4, Line 39: functional_parquet > It would be better to use a temporary db than persistent one like functiona Thanks for your good suggestion. I changed the test. -- 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: 4 Gerrit-Owner: Xiaoqing Gao <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xiaoqing Gao <[email protected]> Gerrit-Comment-Date: Tue, 22 Nov 2022 13:29:35 +0000 Gerrit-HasComments: Yes
