Skip to content
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-34504][SQL] Avoid unnecessary resolving of SQL temp views for DDL commands #31853

Closed
wants to merge 2 commits into from

Conversation

cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

For DDL commands like DROP VIEW, they don't really need to resolve the view (parse and analyze the view SQL text), they just need to get the view metadata.

This PR fixes the rule ResolveTempViews to only resolve the temp view for UnresolvedRelation. This also fixes a bug for DROP VIEW, as previously it tried to resolve the view and failed to drop invalid views.

Why are the changes needed?

bug fix

Does this PR introduce any user-facing change?

no

How was this patch tested?

new test

@@ -1147,16 +1152,10 @@ class Analyzer(override val catalogManager: CatalogManager)
}
// Fail the analysis eagerly because outside AnalysisContext, the unresolved operators
// inside a view maybe resolved incorrectly.
// But for commands like `DropViewCommand`, resolving view is unnecessary even though
// there is unresolved node. So use the `performCheck` flag to skip the analysis check
// for these commands.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This trick doesn't always work. Sometimes the analyzer may fail eagerly instead of waiting for checkAnalysis.

@cloud-fan
Copy link
Contributor Author

cc @linhongliu-db @imback82 @maropu

@@ -950,8 +950,7 @@ class Analyzer(override val catalogManager: CatalogManager)

def lookupTempView(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private?

tmpView
}

def lookupAndResolveTempView(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: private?

Copy link
Member

@maropu maropu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine if the tests pass.

@SparkQA
Copy link

SparkQA commented Mar 16, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40704/

@SparkQA
Copy link

SparkQA commented Mar 16, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40704/

Copy link
Contributor

@imback82 imback82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot added the SQL label Mar 16, 2021
@SparkQA
Copy link

SparkQA commented Mar 16, 2021

Test build #136119 has finished for PR 31853 at commit 5bcb9b5.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Member

maropu commented Mar 16, 2021

Hm, the scala-2.13 build failure seems to be not related to this PR and it is reported in https://issues.apache.org/jira/browse/SPARK-34762.

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Mar 17, 2021

yea the scala 2.13 issue is not related, I'm merging it to master/3.1, thanks for the review!

@cloud-fan cloud-fan closed this in af55373 Mar 17, 2021
cloud-fan added a commit that referenced this pull request Mar 17, 2021
…DDL commands

For DDL commands like DROP VIEW, they don't really need to resolve the view (parse and analyze the view SQL text), they just need to get the view metadata.

This PR fixes the rule `ResolveTempViews` to only resolve the temp view for `UnresolvedRelation`. This also fixes a bug for DROP VIEW, as previously it tried to resolve the view and failed to drop invalid views.

bug fix

no

new test

Closes #31853 from cloud-fan/view-resolve.

Authored-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit af55373)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@linhongliu-db
Copy link
Contributor

late LGTM

flyrain pushed a commit to flyrain/spark that referenced this pull request Sep 21, 2021
…DDL commands

For DDL commands like DROP VIEW, they don't really need to resolve the view (parse and analyze the view SQL text), they just need to get the view metadata.

This PR fixes the rule `ResolveTempViews` to only resolve the temp view for `UnresolvedRelation`. This also fixes a bug for DROP VIEW, as previously it tried to resolve the view and failed to drop invalid views.

bug fix

no

new test

Closes apache#31853 from cloud-fan/view-resolve.

Authored-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit af55373)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
fishcus pushed a commit to fishcus/spark that referenced this pull request Jan 12, 2022
…DDL commands

For DDL commands like DROP VIEW, they don't really need to resolve the view (parse and analyze the view SQL text), they just need to get the view metadata.

This PR fixes the rule `ResolveTempViews` to only resolve the temp view for `UnresolvedRelation`. This also fixes a bug for DROP VIEW, as previously it tried to resolve the view and failed to drop invalid views.

bug fix

no

new test

Closes apache#31853 from cloud-fan/view-resolve.

Authored-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit af55373)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
8 participants