Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 )
Change subject: IMPALA-12935: First pass on Calcite planner functions ...................................................................... Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@33 PS12, Line 33: /** : * A FunctionCallExpr that is always in an analyzed state : */ > Heh, yeah, you got me thinking about this too because this changed from its The part where we immediately analyze as part of the constructor makes for complicated exception handling. RexVisitor doesn't support exceptions, so it adds complication to handle them under those circumstances. I can't really explain why it is necessary. Let me sketch out an alternative: 1. Construct the whole Expr tree without analyzing it 2. Any errors that happen during this process are not usually actionable by the end user. It's good to have a descriptive error message, but it doesn't mean there is something wrong with the SQL. I think that it is ok for this code to throw subclasses of RuntimeException or use Preconditions.checkState() with a good explanation. 3. When we get the Expr tree back in CreateExprVisitor::getExpr(), we call analyze() on the root node, which does a recursive analysis of the whole tree. 4. The special Expr classes don't run analyze() in the constructor, don't keep a reference to the Analyzer, and don't override resetAnalysisState(). They override analyzeImpl() and they should be idempotent. The clone constructor should not need to do anything special, just do a deep copy. I don't want to bog down this review. If we want to address this as a followup, I can live with that, but I don't want us to go too far down this road. (Or if we have a good explanation for why it is necessary, then we can write a good comment and move on.) -- To view, visit http://gerrit.cloudera.org:8080/21357 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Gerrit-Change-Number: 21357 Gerrit-PatchSet: 12 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 05 Jun 2024 21:05:55 +0000 Gerrit-HasComments: Yes
