[ https://issues.apache.org/jira/browse/IMPALA-13136?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17854526#comment-17854526 ]
Joe McDonnell commented on IMPALA-13136: ---------------------------------------- [~scarlin] I'm ok with punting on this for a while. We have a long list of things that need to land, and this is more about code cleanliness than functionality. > Refactor AnalyzedFunctionCallExpr > --------------------------------- > > Key: IMPALA-13136 > URL: https://issues.apache.org/jira/browse/IMPALA-13136 > Project: IMPALA > Issue Type: Improvement > Components: Frontend > Reporter: Steve Carlin > Priority: Major > > Copied from code review: > 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.) -- This message was sent by Atlassian Jira (v8.20.10#820010) --------------------------------------------------------------------- To unsubscribe, e-mail: issues-all-unsubscr...@impala.apache.org For additional commands, e-mail: issues-all-h...@impala.apache.org