[SPARK-47561][SQL] Fix analyzer rule order issues about Alias #45718
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
Contributor
Author
dongjoon-hyun
approved these changes
Mar 26, 2024
Member
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Thank you, @cloud-fan .
Member
|
According to the JIRA Affected Version (4.0.0), I merged this to |
Member
|
Looks good to me. |
cloud-fan
added a commit
that referenced
this pull request
Mar 26, 2024
### What changes were proposed in this pull request? We found two analyzer rule execution order issues in our internal workloads: - `CreateStruct.apply` creates `NamePlaceholder` for unresolved `NamedExpression`. However, with certain rule execution order, the `NamedExpression` may be removed (e.g. remove unnecessary `Alias`) before `NamePlaceholder` is resolved, then `NamePlaceholder` can't be resolved anymore. - UNPIVOT uses `UnresolvedAlias` to wrap `UnresolvedAttribute`. There is a conflict about how to determine the final alias name. If `ResolveAliases` runs first, then `UnresolvedAlias` will be removed and eventually the alias will be `b` for nested column `a.b`. If `ResolveReferences` runs first, then we resolve `a.b` first and then `UnresolvedAlias` will determine the alias as `a.b` not `b`. This PR fixes the two issues - `CreateStruct.apply` should determine the field name immediately if the input is `Alias` - The parser rule for UNPIVOT should follow how we parse SELECT and return `UnresolvedAttribute` directly without the `UnresolvedAlias` wrapper. It's a bit risky to fix the order issue between `ResolveAliases` and `ResolveReferences` as it can change the final query schema, we will save it for later. ### Why are the changes needed? fix unstable analyzer behavior with different rule execution orders. ### Does this PR introduce _any_ user-facing change? Yes, some failed queries can run now. The issue for UNPIVOT only affects the error message. ### How was this patch tested? verified by our internal workloads. The repro query is quite complicated to trigger a certain rule execution order so we won't add tests for it. The fix is quite obvious. ### Was this patch authored or co-authored using generative AI tooling? no Closes #45718 from cloud-fan/rule. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Contributor
Author
|
I backported it to 3.5 directly since there is no merge conflicts. Since it's very hard to hit this bug, I didn't backport it further. I've updated the JIRA ticket as well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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?
We found two analyzer rule execution order issues in our internal workloads:
CreateStruct.applycreatesNamePlaceholderfor unresolvedNamedExpression. However, with certain rule execution order, theNamedExpressionmay be removed (e.g. remove unnecessaryAlias) beforeNamePlaceholderis resolved, thenNamePlaceholdercan't be resolved anymore.UnresolvedAliasto wrapUnresolvedAttribute. There is a conflict about how to determine the final alias name. IfResolveAliasesruns first, thenUnresolvedAliaswill be removed and eventually the alias will bebfor nested columna.b. IfResolveReferencesruns first, then we resolvea.bfirst and thenUnresolvedAliaswill determine the alias asa.bnotb.This PR fixes the two issues
CreateStruct.applyshould determine the field name immediately if the input isAliasUnresolvedAttributedirectly without theUnresolvedAliaswrapper. It's a bit risky to fix the order issue betweenResolveAliasesandResolveReferencesas it can change the final query schema, we will save it for later.Why are the changes needed?
fix unstable analyzer behavior with different rule execution orders.
Does this PR introduce any user-facing change?
Yes, some failed queries can run now. The issue for UNPIVOT only affects the error message.
How was this patch tested?
verified by our internal workloads. The repro query is quite complicated to trigger a certain rule execution order so we won't add tests for it. The fix is quite obvious.
Was this patch authored or co-authored using generative AI tooling?
no