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-37133][SQL] Add a config to optionally enforce ANSI reserved keywords #34403

Closed
wants to merge 4 commits into from

Conversation

cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented Oct 27, 2021

What changes were proposed in this pull request?

This PR adds a new config to optionally enforce the ANSI reserved keywords in the parser. The default value is true, so we by default still enforce it and there is no behavior change.

Why are the changes needed?

In Spark 3.2, the ANSI mode is GA. We want more people to try and use the ANSI mode, to find data issues as early as possible and get better data quality. However, the reserved keywords thing is a big stopper for many users that want to try ANSI mode. They have to update the SQL queries to pass the parser, which is nothing about data quality but just trouble.

With a new config to allow users to not enforce reserved keywords, I think we can get better adoption of the ANSI mode.

Does this PR introduce any user-facing change?

no

How was this patch tested?

updated tests.

@github-actions github-actions bot added the SQL label Oct 27, 2021
@cloud-fan
Copy link
Contributor Author

Copy link
Member

@gengliangwang gengliangwang left a comment

Choose a reason for hiding this comment

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

+1, this helps some users to turn on ANSI mode. For example, a user may have a table named "user" but the table name becomes a reserved keyword if ANSI mode is on.

@SparkQA
Copy link

SparkQA commented Oct 27, 2021

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

@@ -1728,7 +1728,7 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
}

override def visitCurrentLike(ctx: CurrentLikeContext): Expression = withOrigin(ctx) {
if (conf.ansiEnabled) {
if (conf.ansiEnabled && conf.enforceReservedKeywords) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need conf.enforceReservedKeywords since it implies ANSI is enabled.

@SparkQA
Copy link

SparkQA commented Oct 27, 2021

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

@SparkQA
Copy link

SparkQA commented Oct 27, 2021

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

@SparkQA
Copy link

SparkQA commented Oct 27, 2021

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

@SparkQA
Copy link

SparkQA commented Oct 27, 2021

Test build #144663 has finished for PR 34403 at commit 7539217.

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

@SparkQA
Copy link

SparkQA commented Oct 27, 2021

Test build #144667 has finished for PR 34403 at commit 9e6188c.

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

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Looks a good idea.

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

BTW, should we update the doc sql-ref-ansi-compliance.md to mention this config?

@github-actions github-actions bot added the DOCS label Oct 28, 2021
@cloud-fan
Copy link
Contributor Author

the last commit just updates the doc. I'm merging it to master, thanks for the review!

@cloud-fan cloud-fan closed this in cfb96eb Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants