liuyao 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) 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: Preconditions.checkState(other.fnName_ != null); > nit: Could you add a Preconditions check that other.fnName_ != null? Done 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 > never modified... Did I miss anything? Only fnNamePath_ is saved in StmtTableCache, db_, fn_ are not saved, isBuiltin_ and isAnalyzed_ are both false 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. I added the reset() function, but I didn’t call it in FunctionCallExpr#resetAnalysisState for the following reasons: 1. FunctionName is created in some places, only fn_ is set, fnNamePath_ is not set, if reset is called, both fn_ and fnNamePath_ are null, for example, AggregateInfo#createCountDistinctAggExprParam,NormalizeCountStarRule#apply 2. In some places, fnNamePath_ is set, but if reset is called, FunctionName will continue to be used without analysis. -- 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 <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: liuyao <[email protected]> Gerrit-Comment-Date: Tue, 22 Jun 2021 07:44:36 +0000 Gerrit-HasComments: Yes
