Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception ...................................................................... Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/17604/6/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/17604/6/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@144 PS6, Line 144: if (!isFullyQualified()) { : db_ = analyzer.getDefaultDb(); : if (preferBuiltinsDb && builtinDb.containsFunction(fn_)) { : db_ = BuiltinsDb.NAME; : } : } : Preconditions.checkNotNull(db_); : isBuiltin_ = db_.equals(BuiltinsDb.NAME) && : builtinDb.containsFunction(fn_); I think a simpler solution is resolving the read-write contention here, i.e. making each fields being assigned only once. diff --git a/fe/src/main/java/org/apache/impala/analysis/FunctionName.java b/fe/src/main/java/org/apache/impala/analysis/FunctionName.java index f3b3d5d96..288f7cb6e 100644 --- a/fe/src/main/java/org/apache/impala/analysis/FunctionName.java +++ b/fe/src/main/java/org/apache/impala/analysis/FunctionName.java @@ -121,9 +121,10 @@ public class FunctionName { // Resolve the database for this function. Db builtinDb = BuiltinsDb.getInstance(); if (!isFullyQualified()) { - db_ = analyzer.getDefaultDb(); if (preferBuiltinsDb && builtinDb.containsFunction(fn_)) { db_ = BuiltinsDb.NAME; + } else { + db_ = analyzer.getDefaultDb(); } } Preconditions.checkNotNull(db_); So we won't get ephemeral value on db_. This allows the current shallow copy pattern and doesn't require changing other codes. What do you think? http://gerrit.cloudera.org:8080/#/c/17604/5/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/17604/5/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@274 PS5, Line 274: orderByElement.getExpr().collectAll( : Predicates.instanceOf(FunctionCallExpr.class), fnExprs); : } : substituteOrdinalsAndAliases(orderingExprs, "ORDER BY", analyzer); : : for (FunctionCallExpr fn: fnExprs) fn.getFnName().analyze(analyzer); > > I'm not clear about this. You are collecting fnExprs from the I see. So we do need to take care of existing usage pattern on FunctionName. Not sure if we have other places like this that are not revealed by tests. -- To view, visit http://gerrit.cloudera.org:8080/17604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifd77f5a7cd9a674340770233fe26eb5dc26e666a Gerrit-Change-Number: 17604 Gerrit-PatchSet: 6 Gerrit-Owner: liuyao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: liuyao <[email protected]> Gerrit-Comment-Date: Wed, 23 Jun 2021 11:14:42 +0000 Gerrit-HasComments: Yes
