Steve Carlin created IMPALA-13136:
-------------------------------------
Summary: Refactor AnalyzedFunctionCallExpr
Key: IMPALA-13136
URL: https://issues.apache.org/jira/browse/IMPALA-13136
Project: IMPALA
Issue Type: Improvement
Components: Frontend
Reporter: Steve Carlin
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)
