You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GregOwen <gi...@git.apache.org> on 2018/08/21 18:58:06 UTC
[GitHub] spark pull request #22174: [SPARK-22779] Fallback config defaults should beh...
GitHub user GregOwen opened a pull request:
https://github.com/apache/spark/pull/22174
[SPARK-22779] Fallback config defaults should behave like scalar defaults
## What changes were proposed in this pull request?
The value returned by a call to `spark.conf.get(key)` may be specified in one of 3 different ways:
1. By explicitly setting the value of `key` via e.g. `spark.conf.set(key, value)`
1. By the client providing a default value at call-time via e.g. `spark.conf.get(key, clientDefault)`
1. By the ConfigBuilder providing a default value when it is declared via e.g. `SQLConf.buildConf(key).createWithDefault(builderDefault)`
Currently, the order of precedence among these different sources of value is:
1. Explicit value if one was set
1. Otherwise, client default if one was provided
1. Otherwise, builder default
There is currently one exception to this rule: if the builder happened to be declared with a default that is another ConfigBuilder (via e.g. `SQLConf.buildConf(key).fallbackConf(parentConfigBuilder)`), then we check the builder default first (in this case the parent conf and potentially the parent's parent and so on) and then only use the client default if none of the fallback confs had explicit values defined.
From a user perspective, this means that the client default may or may not be used depending on whether the config in question was declared with a scalar default or a fallback conf default, which is usually not obvious. In the case of a fallback conf default, the value returned by `spark.conf.get` may depend on an arbitrary number of parent confs, which seems like it will lead to particularly difficult to diagnose errors.
## How was this patch tested?
Unit test to verify the precedence order for confs with both scalar and fallback conf defaults.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/GregOwen/apache-spark SPARK-22779
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22174.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #22174
----
commit ca4747981f67162d3f937bc83528bcd82431d8c2
Author: Greg Owen <gr...@...>
Date: 2018-08-21T18:34:31Z
Client-provided defaults take precedence over ConfigBuilder defaults
commit 28ea2303982763c328b69a71e03ad7c12fa5eb4c
Author: Greg Owen <gr...@...>
Date: 2018-08-21T18:36:00Z
moved comment
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22174: [SPARK-22779] Fallback config defaults should behave lik...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22174
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22174: [SPARK-22779][SQL] Fallback config defaults should behav...
Posted by GregOwen <gi...@git.apache.org>.
Github user GregOwen commented on the issue:
https://github.com/apache/spark/pull/22174
Closing this for now since it seems like this might break existing workflows
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22174: [SPARK-22779][SQL] Fallback config defaults shoul...
Posted by GregOwen <gi...@git.apache.org>.
Github user GregOwen closed the pull request at:
https://github.com/apache/spark/pull/22174
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22174: [SPARK-22779] Fallback config defaults should behave lik...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22174
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22174: [SPARK-22779] Fallback config defaults should behave lik...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/22174
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22174: [SPARK-22779][SQL] Fallback config defaults shoul...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/22174#discussion_r211727767
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1954,14 +1954,7 @@ class SQLConf extends Serializable with Logging {
entry.valueConverter(defaultValue)
}
}
- Option(settings.get(key)).getOrElse {
- // If the key is not set, need to check whether the config entry is registered and is
- // a fallback conf, so that we can check its parent.
- sqlConfEntries.get(key) match {
- case e: FallbackConfigEntry[_] => getConfString(e.fallback.key, defaultValue)
- case _ => defaultValue
- }
- }
+ Option(settings.get(key)).getOrElse(defaultValue)
--- End diff --
This is a behavior change. We already have a conf that uses this fallback config. See
```
val SQL_STRING_REDACTION_PATTERN =
buildConf("spark.sql.redaction.string.regex")
.doc("Regex to decide which parts of strings produced by Spark contain sensitive " +
"information. When this regex matches a string part, that string part is replaced by a " +
"dummy value. This is currently used to redact the output of SQL explain commands. " +
"When this conf is not set, the value from `spark.redaction.string.regex` is used.")
.fallbackConf(org.apache.spark.internal.config.STRING_REDACTION_PATTERN)
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22174: [SPARK-22779][SQL] Fallback config defaults should behav...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22174
**[Test build #4284 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4284/testReport)** for PR 22174 at commit [`28ea230`](https://github.com/apache/spark/commit/28ea2303982763c328b69a71e03ad7c12fa5eb4c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22174: [SPARK-22779][SQL] Fallback config defaults should behav...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22174
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22174: [SPARK-22779][SQL] Fallback config defaults should behav...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22174
**[Test build #4284 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4284/testReport)** for PR 22174 at commit [`28ea230`](https://github.com/apache/spark/commit/28ea2303982763c328b69a71e03ad7c12fa5eb4c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org