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-40163][SQL] feat: SparkSession.config(Map) #37478

Closed
wants to merge 13 commits into from
Closed

[SPARK-40163][SQL] feat: SparkSession.config(Map) #37478

wants to merge 13 commits into from

Conversation

seunggabi
Copy link
Contributor

@seunggabi seunggabi commented Aug 11, 2022

https://issues.apache.org/jira/browse/SPARK-40163

What changes were proposed in this pull request?

  • as-is
    private fun config(builder: SparkSession.Builder): SparkSession.Builder {
        val map = YamlUtils.read(this::class.java, "spark", Extension.YAML)

        var b = builder
        map.keys.forEach {
            val k = it
            val v = map[k]

            b = when (v) {
                is Long -> b.config(k, v)
                is String -> b.config(k, v)
                is Double -> b.config(k, v)
                is Boolean -> b.config(k, v)
                else -> b
            }
        }

        return b
    }
}
  • to-be
    private fun config(builder: SparkSession.Builder): SparkSession.Builder {
        val map = YamlUtils.read(this::class.java, "spark", Extension.YAML)

        return b.config(map)
    }
}

Why are the changes needed?

  • string, boolean, long, double -> toString
  • so this is simple code!

Does this PR introduce any user-facing change?

How was this patch tested?

  • added test code

@github-actions github-actions bot added the SQL label Aug 11, 2022
@github-actions github-actions bot added the BUILD label Aug 11, 2022
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@seunggabi
Copy link
Contributor Author

I will add .conf test code today.

@seunggabi seunggabi changed the title feat: config(key, value); value by Object [SPARK][SQL] feat: conf(String, Any) Aug 12, 2022
@seunggabi seunggabi requested a review from srowen August 16, 2022 16:59
@srowen
Copy link
Member

srowen commented Aug 16, 2022

This also lacks a JIRA, but, don't bother until we decide whether this can move forward even

@seunggabi
Copy link
Contributor Author

This also lacks a JIRA, but, don't bother until we decide whether this can move forward even

yes I am ready for create JIRA.
If you say to me create JIRA, I create JIRA.

@seunggabi seunggabi requested a review from srowen August 17, 2022 11:31
@srowen
Copy link
Member

srowen commented Aug 17, 2022

Same question as above - isn't this working by just binding to deprecated methods? I don't think this is worth deprecating all the calls apps make now.

@seunggabi
Copy link
Contributor Author

Same question as above - isn't this working by just binding to deprecated methods? I don't think this is worth deprecating all the calls apps make now.

right, so I removed @deprecated. I add just new conf method. (vs config).
maybe I don't understand your last comment.
would you show me to-be ?

@srowen
Copy link
Member

srowen commented Aug 17, 2022

What would the new method bind to now? wouldn't it just bind to the existing methods in almost all cases? so what is the value of the new one?

@seunggabi
Copy link
Contributor Author

seunggabi commented Aug 17, 2022

@srowen
yes. you're right.
as-is works every case.

but, I suggested for flexible code.
worth is flexible code? <- it may be my own idea.

if this pr is closed, I'm OK!
please make a decision (destiny of this PR)


as-is

            b = when (v) {
                is Long -> b.config(k, v)
                is String -> b.config(k, v)
                is Double -> b.config(k, v)
                is Boolean -> b.config(k, v)
                else -> b
            }

to-be

            b = b.conf(it, map[it])

@srowen
Copy link
Member

srowen commented Aug 17, 2022

I see your point, OK. Hm, maybe a more useful overload is a "put all" method that takes a Map?

@seunggabi
Copy link
Contributor Author

@srowen
Thx, I will make map conf method this week.

Please wait for me

Copy link
Contributor Author

@seunggabi seunggabi left a comment

Choose a reason for hiding this comment

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

@srowen
I am done.
please check.

have a nice day.

def config(map: Map[String, Any]): Builder = synchronized {
map.foreach {
kv: (String, Any) => {
options += kv._1 -> kv._2.toString
Copy link
Contributor Author

Choose a reason for hiding this comment

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

choose

  1. type check -> config(String, type) method
  2. foreach -> options += key -> value

I chose 2.
because it is simple code.

@seunggabi seunggabi changed the title [SPARK][SQL] feat: conf(String, Any) [SPARK][SQL] feat: confing(Map) Aug 19, 2022
@seunggabi seunggabi changed the title [SPARK][SQL] feat: confing(Map) [SPARK][SQL] feat: SparkSession.confing(Map) Aug 19, 2022
@srowen
Copy link
Member

srowen commented Aug 20, 2022

Last thing, I'd go ahead and make a JIRA for this. It's minor but non trivial

@seunggabi
Copy link
Contributor Author

thx @srowen
have a nice weekend.!

@srowen
Copy link
Member

srowen commented Aug 20, 2022

Please edit the title to link to the JIRA

@seunggabi
Copy link
Contributor Author

seunggabi commented Aug 20, 2022

@srowen
You mean,
A: I make JIRA, and change title and attach link.(I dont have jira project auth)
B: you give to JIRA, and change title and attach link.

A or B?

I maybe think B.
I will change title and attach link.
B right?

@srowen
Copy link
Member

srowen commented Aug 21, 2022

You can file a JIRA, please

@seunggabi
Copy link
Contributor Author

@srowen
thx for guide, I changed title & attach link!

@srowen srowen changed the title [SPARK][SQL] feat: SparkSession.confing(Map) [SPARK-40163][SPARK][SQL] feat: SparkSession.confing(Map) Aug 21, 2022
@srowen
Copy link
Member

srowen commented Aug 21, 2022

It goes in the title, I added it. (https://spark.apache.org/contributing.html) LGTM

@seunggabi seunggabi changed the title [SPARK-40163][SPARK][SQL] feat: SparkSession.confing(Map) [SPARK-40163][SPARK][SQL] feat: SparkSession.config(Map) Aug 21, 2022
@seunggabi
Copy link
Contributor Author

@srowen
I forget, to click title edit update buttion.
I edited code. (JIRA -> test code comment). caee621

Thank you for spending a lot of time for me as a beginner.
I learned a lot, and I think I can become a better contributor in the future. 🚀

@seunggabi seunggabi changed the title [SPARK-40163][SPARK][SQL] feat: SparkSession.config(Map) [SPARK-40163][SQL] feat: SparkSession.config(Map) Aug 21, 2022
@srowen
Copy link
Member

srowen commented Aug 21, 2022

Merged to master

@srowen srowen closed this in ec4d0ce Aug 21, 2022
@seunggabi seunggabi deleted the feat/spark-session-config-by-object branch August 22, 2022 02:18
srowen pushed a commit that referenced this pull request Sep 2, 2022
… Java `assert` in `JavaSparkSessionSuite.java`

### What changes were proposed in this pull request?
This pr is a minor fix of #37478,  just change to use Junit  api to assert in Java suite.

### Why are the changes needed?
In Java suites, should use JUnit API to make assertions.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass GitHub Actions

Closes #37772 from LuciferYang/SPARK-40163-followup.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants