liuyao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17604 )
Change subject: IMPALA-9763: builtin functions throw unknown exception ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@27 PS3, Line 27: me time. > nit: defaultDb Done http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@28 PS3, Line 28: getDefault > builtin? Done http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@30 PS3, Line 30: FunctionCallExpr::analyzeImpl, and just completed FunctionName::analyze; > I think you can add a summary that there is a read-write contention in Func Done http://gerrit.cloudera.org:8080/#/c/17604/3/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/3/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@173 PS3, Line 173: Name_.clone(); > nit: can we change this to isFnAnalyzed()? It's more readable for me. Yes, the readability here is not good. But I think it might be better to drop the shallow copy and modify the code that relies on the shallow copy. 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 think we don't need to clone this since it's private and never modified. I removed the "shallow copy" in the copy constructor of FunctionCallExpr, so I need to copy fnNamePath_ 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(); > Is it safe to clone the analysis results? E.g. if two dbs both have a foo() 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) at org.apache.impala.analysis.SelectStmt$SelectAnalyzer.analyzeSelectClause(SelectStmt.java:341) at org.apache.impala.analysis.SelectStmt$SelectAnalyzer.analyze(SelectStmt.java:280) at org.apache.impala.analysis.SelectStmt$SelectAnalyzer.access$100(SelectStmt.java:265) at org.apache.impala.analysis.SelectStmt.analyze(SelectStmt.java:258) at org.apache.impala.analysis.AnalysisContext.reAnalyze(AnalysisContext.java:575) at org.apache.impala.analysis.AnalysisContext.analyze(AnalysisContext.java:549) at org.apache.impala.analysis.AnalysisContext.analyzeAndAuthorize(AnalysisContext.java:445) at org.apache.impala.service.Frontend.doCreateExecRequest(Frontend.java:1669) at org.apache.impala.service.Frontend.getTExecRequest(Frontend.java:1635) at org.apache.impala.service.Frontend.createExecRequest(Frontend.java:1605) at org.apache.impala.service.JniFrontend.createExecRequest(JniFrontend.java:163) 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. http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@72 PS3, Line 72: @Override > nit: Could you add an "@Override" annotation? Done -- 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 11:53:52 +0000 Gerrit-HasComments: Yes
