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-29530][SQL][2.4] Make SQLConf in SQL parse process thread safe #26240

Closed
wants to merge 3 commits into from

Conversation

AngersZhuuuu
Copy link
Contributor

What changes were proposed in this pull request?

SparkSession.sql() method parse process not under current sparksession's conf, so some configuration about parser is not valid in multi-thread situation.

In this pr, we add a SQLConf parameter to AbstractSqlParser and initial it with SessionState's conf.
Then for each SparkSession's parser process. It will use's it's own SessionState's SQLConf and to be thread safe

Why are the changes needed?

Fix bug

Does this PR introduce any user-facing change?

NO

How was this patch tested?

NO

@AngersZhuuuu
Copy link
Contributor Author

@gatorsmile Open pr for branch-2.4.

@HyukjinKwon
Copy link
Member

ok to test

@uncleGen
Copy link
Contributor

Could you please add some unit test?

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Oct 24, 2019

Could you please add some unit test?

This pr is to fix problem comment in #26187 (comment)
And back port to branch-2.4

@SparkQA
Copy link

SparkQA commented Oct 24, 2019

Test build #112591 has finished for PR 26240 at commit bc36fb5.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu
Copy link
Contributor Author

@gatorsmile @HyukjinKwon @uncleGen
First test failed because in PlanParserSuit, method withSQLConf will change SQLConf of SQLConf.get() so it's good for origin way. since in ParserDriver it use SQLConf.get.
Nest commit i change like below to fix this problem.

/** For test-only. */
- object CatalystSqlParser extends AbstractSqlParser(new SQLConf()) {
-  val astBuilder = new AstBuilder(new SQLConf())
- }

+ object CatalystSqlParser extends AbstractSqlParser(SQLConf.get) {
+   val astBuilder = new AstBuilder(SQLConf.get)
+ }

@SparkQA
Copy link

SparkQA commented Oct 24, 2019

Test build #112601 has finished for PR 26240 at commit 8dc4047.

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

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-29530][SQL][branch-2.4] Make SQLConf in SQL parse process thread safe [SPARK-29530][SQL][2.4] Make SQLConf in SQL parse process thread safe Oct 24, 2019
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, @AngersZhuuuu and @HyukjinKwon .
Merged to branch-2.4

dongjoon-hyun pushed a commit that referenced this pull request Oct 25, 2019
### What changes were proposed in this pull request?

SparkSession.sql() method parse process not under current sparksession's conf, so some configuration about parser is not valid in multi-thread situation.

In this pr, we add a SQLConf parameter to AbstractSqlParser and initial it with SessionState's conf.
Then for each SparkSession's parser process. It will use's it's own SessionState's SQLConf and to be thread safe

### Why are the changes needed?
Fix bug

### Does this PR introduce any user-facing change?
NO

### How was this patch tested?
NO

Closes #26240 from AngersZhuuuu/SPARK-29530-V2.4.

Authored-by: angerszhu <angers.zhu@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants