Skip to content

[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

Conversation

mihailom-db
Copy link
Contributor

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.

@github-actions github-actions bot added the SQL label Apr 10, 2025
Copy link
Contributor

@vladimirg-db vladimirg-db left a 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!

Copy link
Contributor

@vladimirg-db vladimirg-db left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@@ -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
Copy link
Contributor

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.

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in e00e189 Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants