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 3: (7 comments) Nice finding! Thanks for fixing this bug! I just have some minor 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: defultdb nit: defaultDb http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@28 PS3, Line 28: pure time builtin? http://gerrit.cloudera.org:8080/#/c/17604/3//COMMIT_MSG@30 PS3, Line 30: I think you can add a summary that there is a read-write contention in FunctionName#analyze() when a FunctionName object is shared to two threads: 'isBuiltin_' can be incorrectly evaluated to false when 'db_' is temporarily modified by the other thread. 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: getFunction() != null nit: can we change this to isFnAnalyzed()? It's more readable for me. 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. 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() function but using different implementation, will this cause troubles? FunctionName doesn't have reset() or resetAnalysisState() methods. Although we reset the analysis results after cloning the QueryStmt in constructing an InlineViewRef, the reset doesn't go into this class. If we'd like to keep them, could you add a reset() method in this class and use it in FunctionCallExpr#resetAnalysisState()? http://gerrit.cloudera.org:8080/#/c/17604/3/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@72 PS3, Line 72: public FunctionName clone() { return new FunctionName(this); } nit: Could you add an "@Override" annotation? -- 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: 3 Gerrit-Owner: liuyao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Mon, 21 Jun 2021 07:11:26 +0000 Gerrit-HasComments: Yes
