Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Feb 17, 2021

What changes were proposed in this pull request?

  1. Make the following SQL configs as internal:
    • spark.sql.legacy.allowHashOnMapType
    • spark.sql.legacy.sessionInitWithConfigDefaults
  2. Add a test to check that all SQL configs from the legacy namespace are marked as internal configs.

Why are the changes needed?

Assuming that legacy SQL configs shouldn't be set by users in common cases. The purpose of such configs is to allow switching to old behavior in corner cases. So, the configs should be marked as internals.

Does this PR introduce any user-facing change?

Should not.

How was this patch tested?

By running new test:

$ build/sbt "test:testOnly *SQLConfSuite"

@github-actions github-actions bot added the SQL label Feb 17, 2021
@MaxGekk MaxGekk changed the title [SPARK-34454] Mark legacy SQL configs as internal [SPARK-34454][SQL] Mark legacy SQL configs as internal Feb 17, 2021
@SparkQA
Copy link

SparkQA commented Feb 17, 2021

Test build #135200 has finished for PR 31577 at commit 111635d.

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

@SparkQA
Copy link

SparkQA commented Feb 18, 2021

Test build #135226 has started for PR 31577 at commit 84aed2f.

@SparkQA
Copy link

SparkQA commented Feb 18, 2021

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

@SparkQA
Copy link

SparkQA commented Feb 18, 2021

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

@srowen
Copy link
Member

srowen commented Feb 18, 2021

Seems OK to me if we're saying nobody should be able to set it? but would we want to forbid it?

@MaxGekk
Copy link
Member Author

MaxGekk commented Feb 18, 2021

but would we want to forbid it?

If someone has a good reason to have a legacy conf as non internal, she/he can add an exception to the test. But I believe we should have a mechanism to catch the situation when someone adds "external" legacy config.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @MaxGekk and all.
Yes, I also agree that this is good for consistency and will prevent any surprise in the future.
For the specific case, we can allow it specifically as @MaxGekk suggested.
Merged to master.

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.

6 participants