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 11: (8 comments) http://gerrit.cloudera.org:8080/#/c/9800/11/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java File fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java: http://gerrit.cloudera.org:8080/#/c/9800/11/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@38 PS11, Line 38: @VisibleForTesting Not needed if we test FunctionName directly http://gerrit.cloudera.org:8080/#/c/9800/11/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/11/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@98 PS11, Line 98: * - When preferBuiltinsDb is true, set the database name to current session DB name. need to flip true/false in this comment, might want to start with the true case http://gerrit.cloudera.org:8080/#/c/9800/11/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@122 PS11, Line 122: if (preferBuiltinsDb) { logic needs to be reversed http://gerrit.cloudera.org:8080/#/c/9800/11/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@124 PS11, Line 124: isBuiltin_ = db_.equals(Catalog.BUILTINS_DB); My understanding is isBuiltin_" is true of the function path resolves to a builtin function. So the function name also needs to match a builtin. http://gerrit.cloudera.org:8080/#/c/9800/11/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@134 PS11, Line 134: isBuiltin_ = db_.equals(Catalog.BUILTINS_DB); this case is definitely false since the function name does not match http://gerrit.cloudera.org:8080/#/c/9800/11/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/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@3108 PS11, Line 3108: AnalyzesOk(AnalyzesOk("create function sin(double) RETURNS double" + udfSuffix), This code is really confusing to me. AnalyzeOk() is called twice with completely different semantics. We are not analyzing the statement twice. I think it's simpler to test FunctionName directly without wrapping it in a statement. In my opinion this new test belongs in AnalyzeStmtsTest because that's where we test the analysis of various other paths. See, e.g., TestImplicitAndExplicitPaths() and it's helper functions. We could add a new TestFunctionPaths() that directly tests FunctionName.analyze() http://gerrit.cloudera.org:8080/#/c/9800/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@3899 PS11, Line 3899: private static Function<ParseNode, ParseNode> isBuiltinCreateFunction( This code is sparse and has a lot of unnecessary indirection. Simpler to test FunctionName directly. http://gerrit.cloudera.org:8080/#/c/9800/11/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java: http://gerrit.cloudera.org:8080/#/c/9800/11/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@300 PS11, Line 300: public ParseNode AnalyzesOk(ParseNode node, This extra indirection is unnecessary and confusing. This function is not analyzing anything and it is not asserting "Ok". This function does not do any of the things implied by its name. -- 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: 11 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: Fri, 30 Mar 2018 04:35:18 +0000 Gerrit-HasComments: Yes
