Uh oh!
There was an error while loading. Please reload this page.
[SPARK-27561][SQL][FOLLOWUP] Move the two rules for Later column alias into one file - #39054
[SPARK-27561][SQL][FOLLOWUP] Move the two rules for Later column alias into one file#39054gengliangwang wants to merge 1 commit into
Conversation
gengliangwang
commented
Dec 13, 2022
| /** | ||
| * This rule is the second phase to resolve lateral column alias. | ||
| * The first phase to resolve lateral column alias. |
There was a problem hiding this comment.
It is more clear to put the comment in the first phase rule
There was a problem hiding this comment.
If this is a follow-up for SPARK-27561, the original PR (#39040) is not merged yet. Could you make a PR to him in order to commit as a single patch, @gengliangwang ?
gengliangwang
commented
Dec 14, 2022
@dongjoon-hyun This one is follow-up of #38776. It doesn't contains the code changes of #39040 |
dongjoon-hyun
commented
Dec 14, 2022
Then, let me revise the title of #39040 . |
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Merged to master.
Thank you for clarification, @gengliangwang .
anchovYu
commented
Dec 16, 2022
This PR changes to use the resolver from the SimpleAnalyzer, which is always case sensitive one instead of the one that is determined by conf. This will cause lca unable to resolve case insensitive alias. I fix this issue in the new pr 136a930 by refactoring the code. |
dongjoon-hyun
commented
Dec 16, 2022
Could you make a PR, @anchovYu ? |
…s into one file ### What changes were proposed in this pull request? Move the two rules `WrapLateralColumnAliasReference` and `ResolveLateralColumnAliasReference` into one file `ResolveLateralColumnAlias.scala`, instead of one rule in `Analyzer.scala` and the other one in another file. Also update the code comments. ### Why are the changes needed? Found this issue during reviewing apache#39040 This refactor should make the code easier to read and review. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing UT Closesapache#39054 from gengliangwang/refactorLCA. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
cloud-fan
commented
Dec 20, 2022
Since this PR introduced a regression (case insensitive problem) and it's actually not necessary after my refactor #38888 , I'm reverting it. |
cloud-fan
commented
Dec 20, 2022
Reverted at 52082d3 |
What changes were proposed in this pull request?
Move the two rules
WrapLateralColumnAliasReferenceandResolveLateralColumnAliasReferenceinto one fileResolveLateralColumnAlias.scala, instead of one rule inAnalyzer.scalaand the other one in another file.Also update the code comments.
Why are the changes needed?
Found this issue during reviewing #39040
This refactor should make the code easier to read and review.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing UT