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 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/17604/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/17604/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@173 PS4, Line 173: fnName_ = other.fnName_.clone(); nit: Could you add a Preconditions check that other.fnName_ != null? 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) ? > I removed the "shallow copy" in the copy constructor of FunctionCallExpr, s Sorry, I'm still not clear why we need to clone a field that is never modified... Did I miss anything? 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 FunctionName::fnNamePath_ > is null. > >use functional; >Select count(1) from alltypesagg; > >I0621 19:03:29.787499 46677 jni-util.cc:286] >0649db5f6bc675d6:6b92f3b600000000] java.lang.NullPointerException >at >org.apache.impala.analysis.FunctionCallExpr.isConstantImpl(FunctionCallExpr.java:372) >at org.apache.impala.analysis.Expr.isConstant(Expr.java:1460) >at org.apache.impala.analysis.Expr.computeNumDistinctValues(Expr.java:575) >at org.apache.impala.analysis.Expr.analyze(Expr.java:500) >...... hmm, did you reset fnNamePath_ to null so it cause the NullPointerException? In reset() we only reset the analysis results which are db_, fn_, isBuiltin_ and isAnalyzed_. The fnNamePath_ comes from the parser so should not be reset. > I modified the copy constructor of FunctionCallExpr again. I clone QueryStmt > in the constructor of InlineViewRef. At this time, db_ and fn_ are both null, > and isBuiltin_ and isAnalyzed_ are both false, because only fnNamePath_ is > stored in the stmtTableCache, and no other values are stored. OK, this answers my question that cloning the analysis results are safe, because the source objects are in the StmtTableCache and they are never analyzed. All code paths go here will see 'other' has null db_ , fn_ etc. However, this depends on the caller implementation far away from this class. I think adding a reset() method to reset db_, fn_, isBuiltin_ and isAnalyzed_ help to prevent future bugs. -- 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: 4 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: Mon, 21 Jun 2021 12:46:21 +0000 Gerrit-HasComments: Yes
