[SPARK-14004][SQL] NamedExpressions should have at most one qualifier #11822
Conversation
|
Personally, I had once been quite confused by the fact that I believe it's safe and good to enforce the at-most-one-qualifier constraint at type level unless there do exist valid cases where using multiple qualifiers make sense but haven't been implemented in Spark SQL yet. |
|
Test build #53534 has finished for PR 11822 at commit
|
|
Test build #53540 has finished for PR 11822 at commit
|
liancheng
force-pushed
the
spark-14004-aggressive
branch
from
59241ec to
cbef382
Compare
|
Test build #53601 has finished for PR 11822 at commit
|
| @@ -1438,7 +1438,7 @@ object CleanupAliases extends Rule[LogicalPlan] { | |||
| def trimNonTopLevelAliases(e: Expression): Expression = e match { | |||
| case a: Alias => | |||
| Alias(trimAliases(a.child), a.name)( | |||
| a.exprId, a.qualifiers, a.explicitMetadata, a.isGenerated) | |||
| a.exprId, a.qualifier, a.explicitMetadata, a.isGenerated) | |||
There was a problem hiding this comment.
not related to this PR, but we can simplify this to: a.withNewChildren(trimAliases(a.child) :: Nil)
|
Overall LGTM, I think it's more clear to have |
|
LGTM pending tests. The original signature is from very early on when I thought you could refer to a table both by name and alias. |
|
Test build #53681 has finished for PR 11822 at commit
|
|
Thanks! Merging to master. |
## What changes were proposed in this pull request? This is a more aggressive version of PR apache#11820, which not only fixes the original problem, but also does the following updates to enforce the at-most-one-qualifier constraint: - Renames `NamedExpression.qualifiers` to `NamedExpression.qualifier` - Uses `Option[String]` rather than `Seq[String]` for `NamedExpression.qualifier` Quoted PR description of apache#11820 here: > Current implementations of `AttributeReference.sql` and `Alias.sql` joins all available qualifiers, which is logically wrong. But this implementation mistake doesn't cause any real SQL generation bugs though, since there is always at most one qualifier for any given `AttributeReference` or `Alias`. ## How was this patch tested? Existing tests should be enough. Author: Cheng Lian <lian@databricks.com> Closes apache#11822 from liancheng/spark-14004-aggressive.
What changes were proposed in this pull request?
This is a more aggressive version of PR #11820, which not only fixes the original problem, but also does the following updates to enforce the at-most-one-qualifier constraint:
NamedExpression.qualifierstoNamedExpression.qualifierOption[String]rather thanSeq[String]forNamedExpression.qualifierQuoted PR description of #11820 here:
How was this patch tested?
Existing tests should be enough.