[SPARK-46484][SQL][CONNECT] Make resolveOperators* helper functions keep the plan id
#44462
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zhengruifeng
changed the title
resolveOperators* functions keep the plan idresolveOperators* functions keep the plan id
Contributor
Author
HyukjinKwon
approved these changes
Dec 24, 2023
zhengruifeng
changed the title
resolveOperators* functions keep the plan idresolveOperators* functions keep the plan id
Member
|
I converted it to |
zhengruifeng
changed the title
resolveOperators* functions keep the plan idresolveOperators* helper functions keep the plan id
zhengruifeng
force-pushed
the
sql_res_op_keep
branch
from
f665fd0 to
02cb0bc
Compare
zhengruifeng
changed the title
resolveOperators* helper functions keep the plan idresolveOperators* helper functions keep the plan id
Contributor
Author
|
@cloud-fan I think it is ready for a review |
cloud-fan
reviewed
Dec 28, 2023
| newPlan.copyTagsFrom(self) | ||
| newPlan | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This method resolveOperatorsDownWithPruning is very similar to TreeNode#transformDownWithPruning. Would be good if we can refactor and unify them. cc @beliefer
cloud-fan
approved these changes
Dec 28, 2023
Contributor
Author
|
thanks @cloud-fan and @HyukjinKwon merged to master |
This was referenced Dec 29, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
1, make following helper functions keep the plan id in transformation:
resolveOperatorsDownWithPruningresolveOperatorsUpWithNewOutput2, change the way to keep plan id in
ResolveNaturalAndUsingJoin:before:
after:
3, to verify this fix, this PR also reverts previous tags copying changes in the rules
Why are the changes needed?
we had make following rules keep the plan id:
1,
ResolveNaturalAndUsingJoinin 167bbcaresolveOperatorsUpWithPruning, it set the tagProject.hiddenOutputTaginternally, socopyTagsFrom(only works iftags.isEmpty) inresolveOperatorsUpWithPruningtakes no effect2,
ExtractWindowExpressionsin 185a0a5resolveOperatorsDownWithPruning, which doesn't copy tags3,
WidenSetOperationTypesin 17c206fresolveOperatorsUpWithNewOutput -> transformUpWithNewOutput, which doesn't copy tags4,
ResolvePivotin 1a89bdcresolveOperatorsWithPruning -> resolveOperatorsDownWithPruning, which doesn't copy tags5,
CTESubstitutionin 79d1cderesolveOperatorsDownWithPruningandresolveOperatorsUp -> resolveOperatorsUpWithPruning, the former does't copy tagsBut plan id missing issue still keep popping up (see #44454), so this PR attempt to cover more cases by fixing the helper functions which are used to build the rules
6,
ResolveUnpivotresolveOperatorsWithPruning -> resolveOperatorsDownWithPruning, which doesn't copy tags7,
UnpivotCoercionresolveOperators -> resolveOperatorsWithPruning -> resolveOperatorsDownWithPruning, which doesn't copy tagsDoes this PR introduce any user-facing change?
no
How was this patch tested?
ut
Was this patch authored or co-authored using generative AI tooling?
no