[hotfix][table] Introduced UnresolvedFieldReference & ResolvedFieldReference expressions #8017
Conversation
|
@KurtYoung @twalthr Could you have a look? |
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
| this.name = Preconditions.checkNotNull(name); | ||
| this.resultType = Optional.of(resultType); | ||
| this.resultType = Preconditions.checkNotNull(resultType); | ||
| this.inputIndex = inputIndex; |
There was a problem hiding this comment.
Add check for negative values?
| } | ||
| FieldReferenceExpression that = (FieldReferenceExpression) o; | ||
| return Objects.equals(name, that.name) && Objects.equals(resultType, that.resultType); | ||
| return inputIndex == that.inputIndex && |
There was a problem hiding this comment.
nit: order according to field member order?
|
One minor comment: maybe rename the commit message because |
dawidwys
force-pushed
the
unresolved-field
branch
from
ef55440 to
04b3fa5
Compare
FieldReferenceExpression expressions
dawidwys
force-pushed
the
unresolved-field
branch
from
04b3fa5 to
af99fb1
Compare
|
Thank you @dawidwys , LGTM, I created a JIRA to track blink aggregation Expressions refactor: https://issues.apache.org/jira/browse/FLINK-11983 |
| @Override | ||
| public RexNode visit(Expression other) { | ||
| if (other instanceof ResolvedAggInputReference) { | ||
| if (other instanceof UnresolvedFieldReferenceExpression) { |
There was a problem hiding this comment.
I need some clarification here. AFAIK, the only "unresolved" expressions only exist during the phase we constructing table API in original Flink, right? The unresolved expression never get a chance to translate to Calcite's RexNode. It should be resolved before that or an exception will throw.
Are we changing this now? If we want to change this, it will be a little ambiguous with who are responsible for resolving all these expressions. Not sure "ExpressionVisitor" is the one, since it can see unresolved & resolved expressions in the same time.
There was a problem hiding this comment.
Yes I agree UnresolvedFieldReference cannot be translated into RexNode and also that it should never end up in the Planner. This is also the main purpose of the UnresolvedFieldReference to make this distinction clear.
ExpressionVisitor is just a way to traverse the operation tree. I am currently working on moving all the resolution of expressions into a single place in the API module, as part of FLINK-11884. I agree we should move this resolution here up to the API module as well once it's in place.
There was a problem hiding this comment.
Thanks for the clarification, +1 to "move this resolution here up to the API module"
6 participants
What is the purpose of the change
Splits
FieldReferenceExpressionintoUnresolvedFieldReferenceExpressionandFieldReferenceExpression. The latter should use indices of input and field within input as a reference rather than simply always looking up field by name.Proper use of the FieldReferenceExpression is not part of this PR.
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation