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 14: (4 comments) http://gerrit.cloudera.org:8080/#/c/9800/14/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/14/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@138 PS14, Line 138: isBuiltin_ = db_.equals(Catalog.BUILTINS_DB) && builtinDb.containsFunction(fn_); Hoist this out of the if/else branches to simplify the code. Yes, we'll check builtinDb.containsFunction(fn_) twice in some code paths, but I think simpler code trumps that small extra cost. http://gerrit.cloudera.org:8080/#/c/9800/14/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@151 PS14, Line 151: db_ = fnNamePath_.get(0); I think we need toLowerCase() here otherwise L138 might not work correctly http://gerrit.cloudera.org:8080/#/c/9800/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/9800/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@3735 PS14, Line 3735: public void TestFunctionPaths() throws ImpalaException { Test looks good! Thanks for adding it. http://gerrit.cloudera.org:8080/#/c/9800/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@3739 PS14, Line 3739: Analyzer dummyAnalyzer = parseAndAnalyze("select 1", analysisCtx).getAnalyzer(); Shorter: Analyzer dummyAnalyzer = ((StatementBase)AnalyzesOk("select 1")).getAnalyzer(); No need to create analysisCtx -- 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: 14 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Fri, 30 Mar 2018 16:36:15 +0000 Gerrit-HasComments: Yes
