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 5: (3 comments) > Patch Set 5: > > Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7250/ The failures are inrelated. Rebase may help to fix the test failure. http://gerrit.cloudera.org:8080/#/c/17604/3/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/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@64 PS3, Line 64: fnNamePath_ = (other.getFnNamePath() != null) ? > > Sorry, I'm still not clear why we need to clone a field that is I mean we can just use shallow copy, i.e. "fnNamePath_ = other.getFnNamePath()", because it's private and never modified. Nevermind, it's ok in either ways. http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@66 PS3, Line 66: db_ = other.getDb(); : fn_ = other.getFunction(); : isBuiltin_ = other.isBuiltin(); : isAnalyzed_ = other.isFnAnalyzed(); > > > I tried to do this, but an error occurred. Because OK, so it seems risky to call reset() since we have inconsistent usages on the clone-reset-analyze pattern. This patch reveals that we do have places that don't follow this pattern. Thanks for the explanation! 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 original 'orderByElements_' but not the cloned ones inside 'orderingExprs'. And then analyze these original exprs. Do we require all FunctionName objects being analyzed now? Just concerning that it will introduce a new requirement that may be broken in many other places. -- 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: 5 Gerrit-Owner: liuyao <liu...@sensorsdata.cn> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: liuyao <liu...@sensorsdata.cn> Gerrit-Comment-Date: Wed, 23 Jun 2021 08:15:56 +0000 Gerrit-HasComments: Yes
