[SPARK-24665][PySpark] Use SQLConf in PySpark to manage all sql configs by xuanyuanking · Pull Request #21648 · apache/spark

xuanyuanking · 2018-06-27T05:44:55Z

What changes were proposed in this pull request?

Use SQLConf for PySpark to manage all sql configs, drop all the hard code in config usage.

How was this patch tested?

Existing UT.

HyukjinKwon · 2018-06-27T05:50:19Z

This duplicates codes in Scala side. What's a gain by doing this?

Just want to remove the hard coding as we discussed in #21370 (comment). For the duplication of Scala code, currently I have an idea is just call buildConf and doc in Scala side to register the config and leave its doc, and manage the name also default value in python SQLConf. May I ask your suggestion? :) Thx.

For current approach, It introduces another chunk of codes - 100 addition and it doesn't quite look extendable too. I suggested few possible thoughts below. If that's difficult or impossible, I wouldn't do this for now.

Got it, thanks for your advice.

HyukjinKwon · 2018-06-27T06:01:14Z

Can we do this by wrapping existing SQLConf? We can make them static properties by, for example, this hack

or this hack

Great, thanks for your guidance, I'll take a look at them in detail.

During see the currently implement, maybe we can directly use the SQLConf in SessionState? I do this in last commit 453004a. Please have a look when you have time, thanks :)

HyukjinKwon · 2018-06-27T06:02:57Z

I would also look up the attributes via Py4J and dynamically define the attributes here. It's internal purpose

HyukjinKwon · 2018-06-27T06:03:13Z

I'd make this "private" self._sql_conf

HyukjinKwon · 2018-06-27T06:03:35Z

BTW, does Scala side has such attribute? If no, I won't do it in this way.

Got it, I'll try to do this by wrapping existing SQLConf.

HyukjinKwon · 2018-06-27T06:04:05Z

If this can be replaced by sql_conf access, we don't really need this private function.

Yep, agree.

Thanks, Done in 453004a.

SparkQA · 2018-06-27T06:13:59Z

Test build #92372 has finished for PR 21648 at commit 6a77ed5.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ConfigEntry(object):
class SQLConf(object):

xuanyuanking · 2018-06-30T07:51:47Z

I changed the PR title and description cause maybe we can just use the SQLConf in SessionState, don't need to do a extra wrapping work.

HyukjinKwon · 2018-06-30T07:55:09Z

 """
 return self._jsqlContext

+ def conf(self):


Let's make this _conf with a property.

Thanks, done in 4fc0ae4.

HyukjinKwon · 2018-06-30T08:17:13Z

Seems fine if the tests pass. I (or someone else) should take another look before merging it though.

xuanyuanking · 2018-06-30T08:19:12Z

Got it, thanks again for your advise and guidance.

SparkQA · 2018-06-30T11:39:28Z

Test build #92496 has finished for PR 21648 at commit b816549.

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

SparkQA · 2018-06-30T12:06:25Z

Test build #92497 has finished for PR 21648 at commit 4fc0ae4.

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

HyukjinKwon · 2018-07-02T01:39:37Z


+ @property
+ def _conf(self):
+ """Get SQLConf from current SqlContext"""


nit: Accessor for the JVM SQL-specific configurations

Thanks and done in next commit.

HyukjinKwon

LGTM except for a nit

ueshin · 2018-07-02T01:57:06Z

LGTM for the changes.
I'm just wondering if all config types are supported by the PY4J method call. Do we need some tests for that?

HyukjinKwon · 2018-07-02T03:13:54Z

I roughly double checked most of primitive types like Boolean, Long and String work via Py4J, and some codes related with it in Py4J. Some types like Option[...] will probably just return the Py4J object (via those private helper methods like isReplEagerEvalEnabled).

viirya · 2018-07-02T03:25:18Z

+ @property
+ def _conf(self):
+ """Accessor for the JVM SQL-specific configurations"""
+ return self.sparkSession._jsparkSession.sessionState().conf()


What's the difference between this and SQLContext.getConf ? Can users being confused by two similar APIs?

What's the difference between this and SQLContext.getConf ?
getConf returns the value of Spark SQL conf for given key and we add this _conf wants to access the helper methods in SQLConf. Actually this following the behavior in SQLContext.scala.
spark/sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala
Line 80 in 3c0c2d0
private[sql] defconf:SQLConf= sessionState.conf
Can users being confused by two similar APIs?
Maybe we should add more comments to explain this? AFAIC its more natural than get conf in hard code config key.

Btw, _conf sounds like a property internal to the class/object, why we have a underscore there?

Maybe we should add more comments to explain this? AFAIC its more natural than get conf in hard code config key.
Yeah, I think this is good idea.

This one should be considered as an internal API only to use it to reduce the hardcoded ones and the leading underscore implies it. I think we are fine with it.

Ok. Make sense to me.

Thanks for your advise, I want to change the comments in
"""Accessor for the JVM SQL-specific configurations. This property returns a SQLConf which keep same behavior in scala SQLContext, we mainly use this to call the helper methods in SQLConf. """
Do you think it's reasonable.

I'm fine with it.

I am okay too but let's just leave it. It's private and not a big deal though.

viirya

LGTM

SparkQA · 2018-07-02T06:08:48Z

Test build #92518 has finished for PR 21648 at commit 1f40375.

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

HyukjinKwon · 2018-07-02T06:34:48Z

Merged to master.

xuanyuanking mentioned this pull request Jun 27, 2018
[SPARK-24215][PySpark][Follow Up] Implement eager evaluation for DataFrame APIs in PySpark #21553
Closed

HyukjinKwon reviewed Jun 27, 2018
View reviewed changes

Comment threadpython/pyspark/sql/dataframe.py Outdated
HyukjinKwonJun 27, 2018
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make this "private" self._sql_conf

HyukjinKwon reviewed Jun 27, 2018
View reviewed changes

xuanyuanking added 3 commits June 29, 2018 10:56

Move all core configs into a single class
8a8d059

refactor
453004a

delete property function
b816549

xuanyuanking force-pushed the SPARK-24665 branch from 6a77ed5 to b816549Compare June 30, 2018 07:46

xuanyuanking changed the title ~~[SPARK-24665][PySpark] Add SQLConf in PySpark to manage all sql configs~~[SPARK-24665][PySpark] Use SQLConf in PySpark to manage all sql configsJun 30, 2018

HyukjinKwon reviewed Jun 30, 2018
View reviewed changes

address comments
4fc0ae4

HyukjinKwon reviewed Jul 2, 2018
View reviewed changes

HyukjinKwon approved these changes Jul 2, 2018
View reviewed changes

address comments
1f40375

viirya reviewed Jul 2, 2018
View reviewed changes

viirya approved these changes Jul 2, 2018
View reviewed changes

asfgit closed this in 8f91c69Jul 2, 2018

Uh oh!

Conversation

xuanyuanking commented Jun 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 27, 2018

Uh oh!

xuanyuanking commented Jun 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jun 30, 2018

Uh oh!

xuanyuanking commented Jun 30, 2018

Uh oh!

SparkQA commented Jun 30, 2018

Uh oh!

SparkQA commented Jun 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

ueshin commented Jul 2, 2018

Uh oh!

HyukjinKwon commented Jul 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuanyuanking commented Jun 27, 2018•
edited
Loading