You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by vinodkc <gi...@git.apache.org> on 2017/08/05 07:06:26 UTC
[GitHub] spark pull request #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
GitHub user vinodkc opened a pull request:
https://github.com/apache/spark/pull/18852
[SPARK-21588][SQL] SQLContext.getConf(key, null) should return null
## What changes were proposed in this pull request?
In SQLContext.get(key,null) for a key that is not defined in the conf, and doesn't have a default value defined, throws a NPE. In happens only when conf has a value converter
Added null check on defaultValue inside SQLConf.getConfString to avoid calling entry.valueConverter(defaultValue)
## How was this patch tested?
Added unit test
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vinodkc/spark br_Fix_SPARK-21588
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/18852.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 #18852
----
commit 53b73ed21a028ea1915df8a9a03d6d9368e53c8a
Author: vinodkc <vi...@gmail.com>
Date: 2017-08-05T06:39:04Z
SQLContext.getConf(key, null) should return null
----
---
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131527017
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala ---
@@ -270,4 +270,10 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
val e2 = intercept[AnalysisException](spark.conf.unset(SCHEMA_STRING_LENGTH_THRESHOLD.key))
assert(e2.message.contains("Cannot modify the value of a static config"))
}
+
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
+ assert(null == spark.conf.get("spark.sql.thriftServer.incrementalCollect", null))
+ assert("<undefined>" == spark.conf.get(
+ "spark.sql.thriftServer.incrementalCollect", "<undefined>"))
--- End diff --
The test cases need to be improved.
```Scala
withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "1") {
assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, null))
assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, "<undefined>"))
}
assert(spark.conf.getOption("spark.sql.nonexistent").isEmpty)
assert(null == spark.conf.get("spark.sql.nonexistent", null))
assert("<undefined>" == spark.conf.get("spark.sql.nonexistent", "<undefined>"))
```
---
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131534222
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala ---
@@ -270,4 +270,15 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
val e2 = intercept[AnalysisException](spark.conf.unset(SCHEMA_STRING_LENGTH_THRESHOLD.key))
assert(e2.message.contains("Cannot modify the value of a static config"))
}
+
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
+ withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "1") {
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, null))
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, "<undefined>"))
+ }
+
+ assert(spark.conf.getOption("spark.sql.nonexistent").isEmpty)
+ assert(null == spark.conf.get("spark.sql.nonexistent", null))
--- End diff --
This is to improve the test case coverage.
---
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/18852
LGTM pending Jenkins.
---
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18852
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/80283/
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18852
**[Test build #80283 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80283/testReport)** for PR 18852 at commit [`53b73ed`](https://github.com/apache/spark/commit/53b73ed21a028ea1915df8a9a03d6d9368e53c8a).
* 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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131534323
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala ---
@@ -270,4 +270,15 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
val e2 = intercept[AnalysisException](spark.conf.unset(SCHEMA_STRING_LENGTH_THRESHOLD.key))
assert(e2.message.contains("Cannot modify the value of a static config"))
}
+
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
+ withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "1") {
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, null))
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, "<undefined>"))
+ }
+
+ assert(spark.conf.getOption("spark.sql.nonexistent").isEmpty)
+ assert(null == spark.conf.get("spark.sql.nonexistent", null))
--- End diff --
Aha, right. Sorry. Miss it.
---
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/18852
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by vinodkc <gi...@git.apache.org>.
Github user vinodkc commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131519921
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala ---
@@ -808,6 +808,12 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
Row("1"))
}
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
--- End diff --
Thanks for the review comment.
Moved the test to SQLConfSuite
---
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131534275
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala ---
@@ -270,4 +270,15 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
val e2 = intercept[AnalysisException](spark.conf.unset(SCHEMA_STRING_LENGTH_THRESHOLD.key))
assert(e2.message.contains("Cannot modify the value of a static config"))
}
+
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
+ withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "1") {
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, null))
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, "<undefined>"))
+ }
+
+ assert(spark.conf.getOption("spark.sql.nonexistent").isEmpty)
+ assert(null == spark.conf.get("spark.sql.nonexistent", null))
--- End diff --
Yeah, those tests are good. We should add another test for NPE case too.
---
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18852
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/80292/
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18852
**[Test build #80292 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80292/testReport)** for PR 18852 at commit [`4295cd3`](https://github.com/apache/spark/commit/4295cd3342178285c2465c080cafa8dd8b68ba16).
---
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131534235
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala ---
@@ -270,4 +270,15 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
val e2 = intercept[AnalysisException](spark.conf.unset(SCHEMA_STRING_LENGTH_THRESHOLD.key))
assert(e2.message.contains("Cannot modify the value of a static config"))
}
+
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
+ withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "1") {
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, null))
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, "<undefined>"))
+ }
+
+ assert(spark.conf.getOption("spark.sql.nonexistent").isEmpty)
+ assert(null == spark.conf.get("spark.sql.nonexistent", null))
--- End diff --
That's good. But with current test, we don't actually test against the case of NPE which is the main issue.
---
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18852
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/80287/
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/18852
Thanks! Merging to master/2.2/2.1
---
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/18852
---
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18852
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131534140
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala ---
@@ -270,4 +270,15 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
val e2 = intercept[AnalysisException](spark.conf.unset(SCHEMA_STRING_LENGTH_THRESHOLD.key))
assert(e2.message.contains("Cannot modify the value of a static config"))
}
+
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
+ withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "1") {
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, null))
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, "<undefined>"))
+ }
+
+ assert(spark.conf.getOption("spark.sql.nonexistent").isEmpty)
+ assert(null == spark.conf.get("spark.sql.nonexistent", null))
--- End diff --
Because the key doesn't exist, this doesn't actually test the issue. This line passes without this change too.
---
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131534296
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala ---
@@ -270,4 +270,15 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
val e2 = intercept[AnalysisException](spark.conf.unset(SCHEMA_STRING_LENGTH_THRESHOLD.key))
assert(e2.message.contains("Cannot modify the value of a static config"))
}
+
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
+ withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "1") {
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, null))
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, "<undefined>"))
+ }
+
+ assert(spark.conf.getOption("spark.sql.nonexistent").isEmpty)
+ assert(null == spark.conf.get("spark.sql.nonexistent", null))
--- End diff --
The line 276 does not verify the fix? Why we still need to add another test case?
---
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131534246
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala ---
@@ -270,4 +270,15 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
val e2 = intercept[AnalysisException](spark.conf.unset(SCHEMA_STRING_LENGTH_THRESHOLD.key))
assert(e2.message.contains("Cannot modify the value of a static config"))
}
+
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
+ withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "1") {
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, null))
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, "<undefined>"))
+ }
+
+ assert(spark.conf.getOption("spark.sql.nonexistent").isEmpty)
+ assert(null == spark.conf.get("spark.sql.nonexistent", null))
--- End diff --
I knew it, but I just do not want to introduce any regression. Thus, I just to cover both scenarios.
---
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131534217
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala ---
@@ -270,4 +270,15 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
val e2 = intercept[AnalysisException](spark.conf.unset(SCHEMA_STRING_LENGTH_THRESHOLD.key))
assert(e2.message.contains("Cannot modify the value of a static config"))
}
+
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
+ withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "1") {
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, null))
+ assert("1" == spark.conf.get(SQLConf.SHUFFLE_PARTITIONS.key, "<undefined>"))
+ }
+
+ assert(spark.conf.getOption("spark.sql.nonexistent").isEmpty)
+ assert(null == spark.conf.get("spark.sql.nonexistent", null))
--- End diff --
The NPE is only happened for an existing entry like the above `SHUFFLE_PARTITIONS` or the previous `spark.sql.thriftServer.incrementalCollect`.
---
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18852
**[Test build #80292 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80292/testReport)** for PR 18852 at commit [`4295cd3`](https://github.com/apache/spark/commit/4295cd3342178285c2465c080cafa8dd8b68ba16).
* 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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18852
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18852
**[Test build #80287 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80287/testReport)** for PR 18852 at commit [`b4055e1`](https://github.com/apache/spark/commit/b4055e11b25c0c7cd83f34097425391abd5fe9b4).
---
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/18852
LGTM except a comment in test cases.
---
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 #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/18852#discussion_r131517530
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala ---
@@ -808,6 +808,12 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
Row("1"))
}
+ test("SPARK-21588 SQLContext.getConf(key, null) should return null") {
--- End diff --
Maybe directly test against `SQLConf` in `SQLConfSuite`, instead of via `SQLContext`?
---
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18852
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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18852
**[Test build #80287 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80287/testReport)** for PR 18852 at commit [`b4055e1`](https://github.com/apache/spark/commit/b4055e11b25c0c7cd83f34097425391abd5fe9b4).
* 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 issue #18852: [SPARK-21588][SQL] SQLContext.getConf(key, null) should ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18852
**[Test build #80283 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80283/testReport)** for PR 18852 at commit [`53b73ed`](https://github.com/apache/spark/commit/53b73ed21a028ea1915df8a9a03d6d9368e53c8a).
---
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