You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rxin <gi...@git.apache.org> on 2017/12/13 22:45:16 UTC
[GitHub] spark pull request #19973: [SPARK-22779] ConfigEntry's default value should ...
GitHub user rxin opened a pull request:
https://github.com/apache/spark/pull/19973
[SPARK-22779] ConfigEntry's default value should actually be a value
## What changes were proposed in this pull request?
ConfigEntry's config value right now shows a human readable message. In some places in SQL we actually rely on default value for real to be setting the values.
## How was this patch tested?
Tested manually.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rxin/spark SPARK-22779
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19973.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 #19973
----
commit 385c300c14a382654c2a1f94ccd2813487dbe26a
Author: Reynold Xin <rx...@databricks.com>
Date: 2017-12-13T22:43:55Z
[SPARK-22779] ConfigEntry's default value should actually be a value
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] ConfigEntry's default value should actuall...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/19973
The issue is in
```
/**
* Return the `string` value of Spark SQL configuration property for the given key. If the key is
* not set yet, return `defaultValue`.
*/
def getConfString(key: String, defaultValue: String): String = {
if (defaultValue != null && defaultValue != "<undefined>") {
val entry = sqlConfEntries.get(key)
if (entry != null) {
// Only verify configs in the SQLConf object
entry.valueConverter(defaultValue)
}
}
Option(settings.get(key)).getOrElse(defaultValue)
}
```
The value converter gets applied on this generated string which is not a real value and will fail.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] FallbackConfigEntry's default value should...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/19973
That's what the "default" is, isn't it?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] FallbackConfigEntry's default value should...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/19973
@vanzin you got a min to submit a patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] ConfigEntry's default value should actuall...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19973
LGTM. Can you say `FallbackConfigEntry` in the title instead?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] ConfigEntry's default value should actuall...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19973
Actually... is this right? The `defaultValueString` will now be the default value of the parent conf, not its current value...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] FallbackConfigEntry's default value should...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19973
Let me take a look...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] FallbackConfigEntry's default value should...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19973
**[Test build #84882 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84882/testReport)** for PR 19973 at commit [`385c300`](https://github.com/apache/spark/commit/385c300c14a382654c2a1f94ccd2813487dbe26a).
* This patch **fails Spark unit 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 #19973: [SPARK-22779] ConfigEntry's default value should actuall...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19973
**[Test build #84882 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84882/testReport)** for PR 19973 at commit [`385c300`](https://github.com/apache/spark/commit/385c300c14a382654c2a1f94ccd2813487dbe26a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] FallbackConfigEntry's default value should...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19973
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84882/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] FallbackConfigEntry's default value should...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19973
@rxin we can close this now right?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] FallbackConfigEntry's default value should...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19973
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] FallbackConfigEntry's default value should...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19973
Well, the default for fallback configs is the current value of the parent conf - it needs context. The code you reference in the a previous comment has that context (the SQL conf map), so the value of the parent conf might not be the default.
If you look at `ConfigReader.getOrDefault` it has some (kinda nasty) code to do this.
If I understand correctly, the problem is really here (the other `getConfString` method):
```
def getConfString(key: String): String = {
Option(settings.get(key)).
orElse {
// Try to use the default value
Option(sqlConfEntries.get(key)).map(_.defaultValueString)
}.
getOrElse(throw new NoSuchElementException(key))
}
```
That will be broken for fallback configs with the existing code, but it will also return the wrong thing with your updated code, in the case where the parent conf is set.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19973: [SPARK-22779] FallbackConfigEntry's default value...
Posted by rxin <gi...@git.apache.org>.
Github user rxin closed the pull request at:
https://github.com/apache/spark/pull/19973
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19973: [SPARK-22779] ConfigEntry's default value should actuall...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/19973
cc @vanzin @gatorsmile
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org