You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by bomeng <gi...@git.apache.org> on 2016/03/08 01:36:49 UTC
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
GitHub user bomeng opened a pull request:
https://github.com/apache/spark/pull/11568
[SPARK-13727] [SQL] SparkConf.contains does not consider deprecated keys
## What changes were proposed in this pull request?
The contains() method does not return consistently with get() if the key is deprecated. For example,
import org.apache.spark.SparkConf
val conf = new SparkConf()
conf.set("spark.io.compression.lz4.block.size", "12345") /* display some deprecated warning message */
conf.get("spark.io.compression.lz4.block.size") /* return 12345 */
conf.get("spark.io.compression.lz4.blockSize") /* return 12345 */
conf.contains("spark.io.compression.lz4.block.size") /* return true */
**conf.contains("spark.io.compression.lz4.blockSize") /* return false */**
The fix will make the contains() and get() more consistent.
## How was this patch tested?
I've added a test case for this.
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
Unit tests should be sufficient.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bomeng/spark SPARK-13727
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/11568.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 #11568
----
commit 91333f9b1c267a41b71b843b40ebaefc5e60e215
Author: bomeng <bm...@us.ibm.com>
Date: 2016-03-08T00:25:38Z
fix SPARK-13727 with unit test
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-195006855
(BTW I'll fix the title during merge since this is not a sql change.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11568#discussion_r55624429
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -351,7 +351,13 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
def getAppId: String = get("spark.app.id")
/** Does the configuration contain a given parameter? */
- def contains(key: String): Boolean = settings.containsKey(key)
+ def contains(key: String): Boolean = {
+ settings.containsKey(key) || {
+ // try to find the settings in the alternatives
+ val alts = configsWithAlternatives.get(key)
+ alts.isDefined && alts.get.exists { alt => contains(alt.key) }
--- End diff --
just do
```
alts.getOrElse(Seq()).exists { alt => contains(alt.key) }
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski commented on a diff in the pull request:
https://github.com/apache/spark/pull/11568#discussion_r55408473
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -351,7 +351,16 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
def getAppId: String = get("spark.app.id")
/** Does the configuration contain a given parameter? */
- def contains(key: String): Boolean = settings.containsKey(key)
+ def contains(key: String): Boolean = {
+ if (settings.containsKey(key)) {
+ true
+ } else {
+ // try to find the settings in the alternatives
+ configsWithAlternatives.get(key).flatMap { alts =>
+ alts.collectFirst { case alt if contains(alt.key) => true }
+ }.isDefined
--- End diff --
It's far too complicated. Would `configsWithAlternatives.get("one").contains(...)` work here? What is this supposed to do?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-193968072
@srowen thanks for the suggestion. i've pushed the changes to make it more concise.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194627352
Made some changes based on @andrewor14 comments. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/11568#discussion_r55418220
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -351,7 +351,16 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
def getAppId: String = get("spark.app.id")
/** Does the configuration contain a given parameter? */
- def contains(key: String): Boolean = settings.containsKey(key)
+ def contains(key: String): Boolean = {
+ if (settings.containsKey(key)) {
+ true
+ } else {
+ // try to find the settings in the alternatives
+ configsWithAlternatives.get(key).flatMap { alts =>
+ alts.collectFirst { case alt if contains(alt.key) => true }
+ }.isDefined
--- End diff --
OK what about `...flatMap(_.find(contains(_.key)))...`? maybe that's too terse. The case statement mapping to true seemed unnecessary. If I'm still missing something, pardon, just do what you see fit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194659015
**[Test build #52803 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/52803/consoleFull)** for PR 11568 at commit [`eede12b`](https://github.com/apache/spark/commit/eede12b7f40ac4b02bc30c81a8ac4bf1ffefe8e3).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194644473
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/11568#discussion_r55424521
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -351,7 +351,13 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
def getAppId: String = get("spark.app.id")
/** Does the configuration contain a given parameter? */
- def contains(key: String): Boolean = settings.containsKey(key)
+ def contains(key: String): Boolean = {
+ settings.containsKey(key) || {
+ // try to find the settings in the alternatives
+ val alts = configsWithAlternatives.get(key)
+ if (alts.isDefined) alts.get.exists { alt => contains(alt.key) } else false
--- End diff --
I like this better; can it just be `alts.isDefined && alts.get.exists ...`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-193527447
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-193939241
@jaceklaskowski @srowen I've some changes. How about the logic this time?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194663017
**[Test build #52808 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/52808/consoleFull)** for PR 11568 at commit [`6061b86`](https://github.com/apache/spark/commit/6061b860a501166ab987119b1d187c971a17c52b).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/11568
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-195006609
Ok, merging to master. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11568#discussion_r55419451
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -351,7 +351,16 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
def getAppId: String = get("spark.app.id")
/** Does the configuration contain a given parameter? */
- def contains(key: String): Boolean = settings.containsKey(key)
+ def contains(key: String): Boolean = {
+ if (settings.containsKey(key)) {
+ true
+ } else {
+ // try to find the settings in the alternatives
+ configsWithAlternatives.get(key).flatMap { alts =>
+ alts.collectFirst { case alt if contains(alt.key) => true }
+ }.isDefined
--- End diff --
I've provided another approach in my latest code I pushed. It avoids the flatMap and use exist to check. Can you take a look? Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194627656
**[Test build #52808 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/52808/consoleFull)** for PR 11568 at commit [`6061b86`](https://github.com/apache/spark/commit/6061b860a501166ab987119b1d187c971a17c52b).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194663608
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/52808/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194617476
ok to test @vanzin
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194659162
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/52803/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski commented on a diff in the pull request:
https://github.com/apache/spark/pull/11568#discussion_r55406003
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -351,7 +351,16 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
def getAppId: String = get("spark.app.id")
/** Does the configuration contain a given parameter? */
- def contains(key: String): Boolean = settings.containsKey(key)
+ def contains(key: String): Boolean = {
+ if (settings.containsKey(key)) {
--- End diff --
It always bothers me when I see `if (true) true`. I think `settings.containsKey(key) || ...`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194618069
**[Test build #52803 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/52803/consoleFull)** for PR 11568 at commit [`eede12b`](https://github.com/apache/spark/commit/eede12b7f40ac4b02bc30c81a8ac4bf1ffefe8e3).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/11568#discussion_r55407249
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -351,7 +351,16 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
def getAppId: String = get("spark.app.id")
/** Does the configuration contain a given parameter? */
- def contains(key: String): Boolean = settings.containsKey(key)
+ def contains(key: String): Boolean = {
+ if (settings.containsKey(key)) {
+ true
+ } else {
+ // try to find the settings in the alternatives
+ configsWithAlternatives.get(key).flatMap { alts =>
--- End diff --
this is pretty complicated. Is this not just `configsWithAlternatives.get(key).find(contains(_.key)).isDefined` ? or something 90% like that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11568#discussion_r55413797
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -351,7 +351,16 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
def getAppId: String = get("spark.app.id")
/** Does the configuration contain a given parameter? */
- def contains(key: String): Boolean = settings.containsKey(key)
+ def contains(key: String): Boolean = {
+ if (settings.containsKey(key)) {
+ true
+ } else {
+ // try to find the settings in the alternatives
+ configsWithAlternatives.get(key).flatMap { alts =>
+ alts.collectFirst { case alt if contains(alt.key) => true }
+ }.isDefined
--- End diff --
1. It will find the alternatives using the key: configsWithAlternatives.get(key), it will return an Option of Seq, since alternative could be multiple;
2. It will then try to use the key of each alternative to see if it is already defined in the conf, here contains(alt.key) is a recursive call;
3. collectFirst will just get the first matching
4. the final step isDefined() is to see whether we find such a match or not.
Remember configsWithAlternatives.get("one") returns Option of Seq[], any suggestion to simplify?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194663605
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11568#discussion_r55409812
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -351,7 +351,16 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
def getAppId: String = get("spark.app.id")
/** Does the configuration contain a given parameter? */
- def contains(key: String): Boolean = settings.containsKey(key)
+ def contains(key: String): Boolean = {
+ if (settings.containsKey(key)) {
+ true
+ } else {
+ // try to find the settings in the alternatives
+ configsWithAlternatives.get(key).flatMap { alts =>
--- End diff --
configsWithAlternatives.get(key) will return Option[Seq[]], so find won't apply directly. Let me try to find a better way. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13727] [SQL] SparkConf.contains does no...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11568#issuecomment-194659161
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org