[
https://issues.apache.org/jira/browse/IMPALA-13136?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Steve Carlin resolved IMPALA-13136.
-----------------------------------
Resolution: Fixed
> 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)
