Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9800 )
Change subject: IMPALA-6724: Allow creating/dropping functions with the same name as built-ins ...................................................................... Patch Set 9: (6 comments) http://gerrit.cloudera.org:8080/#/c/9800/9/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/9800/9/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@92 PS9, Line 92: * Function resolution happens as follows. I would say "Path resolution happens as follows" because we use that "Path" terminology elsewhere and "Function resolution" could be misunderstood to mean resolving a function including its signature. http://gerrit.cloudera.org:8080/#/c/9800/9/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@104 PS9, Line 104: public void analyze(Analyzer analyzer, boolean useDefaultDb) throws AnalysisException { This is more of a nit, but I think a cleaner name for the variable is something like "useBuiltinsDb" or "preferBuiltinsDb" or "assumeBuiltinsDb" It's not clear from "useDefaultDb" what will happen when it is false without reading the method comment. Assuming the session database is the common behavior, so I think it's clearer to signify the non-common behavior with the name of this new function argument. http://gerrit.cloudera.org:8080/#/c/9800/9/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@123 PS9, Line 123: isBuiltin_ = false; Not sure if we use isBuiltin_ for anything significant, but I don't think this code path implies that the path refers to a non-builtin. The user could have done: USE _impala_builtins; CREATE FUNCTION sin(); Yes, the CREATE FUNCTION will fail, but this FunctionName will analyze successfully and have isBuiltin_ false. Can we write a test that would have caught this? http://gerrit.cloudera.org:8080/#/c/9800/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/9800/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@3109 PS9, Line 3109: AnalysisError("create function _impala_builtins.sin(double) returns double" + Also add a test where "_impala_builtins" is the session database http://gerrit.cloudera.org:8080/#/c/9800/9/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test File testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test: http://gerrit.cloudera.org:8080/#/c/9800/9/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test@312 PS9, Line 312: # Query a custom sin function. Combine this and the following query: SELECT $DATABASE.sin(0), sin(0); http://gerrit.cloudera.org:8080/#/c/9800/9/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test@353 PS9, Line 353: # Query a custom sin function. Combine this and the following query. -- To view, visit http://gerrit.cloudera.org:8080/9800 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic30df56ac276970116715c14454a5a2477b185fa Gerrit-Change-Number: 9800 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 29 Mar 2018 23:23:23 +0000 Gerrit-HasComments: Yes
