-
Notifications
You must be signed in to change notification settings - Fork 28.5k
[SPARK-51762][SQL] Fix Resolution of Views in Single-Pass Analyzer When Bridging is Enabled #50555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-51762][SQL] Fix Resolution of Views in Single-Pass Analyzer When Bridging is Enabled #50555
Conversation
sql/core/src/test/scala/org/apache/spark/sql/analysis/resolver/MetadataResolverSuite.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/analysis/resolver/MetadataResolverSuite.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ViewResolver.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ViewResolver.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ViewResolver.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/Resolver.scala
Outdated
Show resolved
Hide resolved
.../scala/org/apache/spark/sql/catalyst/analysis/resolver/BridgedRelationMetadataProvider.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after resolving comments. Thanks for this change!
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
Outdated
Show resolved
Hide resolved
...alyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/BridgedRelationId.scala
Show resolved
Hide resolved
.../scala/org/apache/spark/sql/catalyst/analysis/resolver/BridgedRelationMetadataProvider.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/Resolver.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/Resolver.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ViewResolver.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ViewResolver.scala
Outdated
Show resolved
Hide resolved
.../scala/org/apache/spark/sql/catalyst/analysis/resolver/BridgedRelationMetadataProvider.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after resolving the comment
@@ -101,14 +108,16 @@ class ViewResolver(resolver: Resolver, catalogManager: CatalogManager) | |||
val prevContext = if (viewResolutionContextStack.isEmpty()) { | |||
ViewResolutionContext( | |||
nestedViewDepth = 0, | |||
maxNestedViewDepth = conf.maxNestedViewDepth | |||
maxNestedViewDepth = conf.maxNestedViewDepth, | |||
catalogAndNamespace = unresolvedView.desc.viewCatalogAndNamespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed, right? The copy
below is always called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, true. But we wanted to make sure that catalogAndNamespace are always provided on construction of the ViewResolutionContext
. We can only combine the copy with this if statement. But that seems to make the code less readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplication will quickly get annoying, let's not do that.
...alyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/BridgedRelationId.scala
Outdated
Show resolved
Hide resolved
@@ -101,14 +108,16 @@ class ViewResolver(resolver: Resolver, catalogManager: CatalogManager) | |||
val prevContext = if (viewResolutionContextStack.isEmpty()) { | |||
ViewResolutionContext( | |||
nestedViewDepth = 0, | |||
maxNestedViewDepth = conf.maxNestedViewDepth | |||
maxNestedViewDepth = conf.maxNestedViewDepth, | |||
catalogAndNamespace = unresolvedView.desc.viewCatalogAndNamespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplication will quickly get annoying, let's not do that.
…ual' into fixViewResolutionInDual
thanks, merging to master! |
What changes were proposed in this pull request?
Fix for Dual run of Single-Pass Analyzer with Bridging enabled.
Why are the changes needed?
SPARK-30284: CREATE VIEW should track the current catalog and namespace
test was failing when Bridging is enabled.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Fixing specific tests. Tests run locally.
Was this patch authored or co-authored using generative AI tooling?
No.