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-20204][SQL][Followup] SQLConf should react to change in default timezone settings #17537
Conversation
def createWithDefaultFunction(defaultFunc: () => T): ConfigEntry[T] = { | ||
val entry = | ||
new ConfigEntryWithDefaultFunction[T](parent.key, defaultFunc, converter, | ||
stringConverter, parent._doc, parent._public) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
val entry = new XXX(
param1, param2, ...)
LGTM, we can have a unit test to make sure the default value can change after the conf entry is created, in |
@cloud-fan Sure. I will add the unit test. |
LGTM for the current change. Look forward for the test to add. |
@@ -51,6 +52,26 @@ class ConfigEntrySuite extends SparkFunSuite { | |||
assert(conf.get(dConf) === 20.0) | |||
} | |||
|
|||
test("conf entry: timezone") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are testing ConfigEntryWithDefaultFunction
, not timezone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also move this test to the end, it's weird to put this test in the middle of different config types tests.
@@ -51,6 +52,26 @@ class ConfigEntrySuite extends SparkFunSuite { | |||
assert(conf.get(dConf) === 20.0) | |||
} | |||
|
|||
test("conf entry: timezone") { | |||
val tzStart = TimeZone.getDefault().getID() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test can be as simple as:
var data = 0
val conf = new SparkConf()
val iConf = ConfigBuilder(testKey("int")).intConf.createWithDefaultFunction(() => data)
assert(conf.get(iConf) === 0)
data = 2
assert(conf.get(iConf) === 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloud-fan Thank you :-). Its so much simpler. I somehow was trying to use the exact same function that we use in the code.
Test build #75537 has finished for PR 17537 at commit
|
Test build #75540 has finished for PR 17537 at commit
|
@@ -147,6 +147,14 @@ private[spark] class TypedConfigBuilder[T]( | |||
} | |||
} | |||
|
|||
/** Creates a [[ConfigEntry]] with a function has a default value */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creates a [[ConfigEntry]] with a function has a default value
->
Creates a [[ConfigEntry]] with a function to determine the default value
@@ -17,6 +17,7 @@ | |||
|
|||
package org.apache.spark.internal.config | |||
|
|||
import java.util.TimeZone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove it?
@@ -51,6 +52,8 @@ class ConfigEntrySuite extends SparkFunSuite { | |||
assert(conf.get(dConf) === 20.0) | |||
} | |||
|
|||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: two more extra spaces
@@ -752,7 +752,7 @@ object SQLConf { | |||
buildConf("spark.sql.session.timeZone") | |||
.doc("""The ID of session local timezone, e.g. "GMT", "America/Los_Angeles", etc.""") | |||
.stringConf | |||
.createWithDefault(TimeZone.getDefault().getID()) | |||
.createWithDefaultFunction(() => TimeZone.getDefault().getID()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TimeZone.getDefault().getID()
->
TimeZone.getDefault.getID
LGTM with minor comments. |
Test build #75548 has finished for PR 17537 at commit
|
LGTM pending Jenkins |
Test build #75558 has finished for PR 17537 at commit
|
LGTM |
thanks, merging to master! |
@viirya @cloud-fan @gatorsmile Thanks a lot. |
What changes were proposed in this pull request?
Make sure SESSION_LOCAL_TIMEZONE reflects the change in JVM's default timezone setting. Currently several timezone related tests fail as the change to default timezone is not picked up by SQLConf.
How was this patch tested?
Added an unit test in ConfigEntrySuite