Skip to content

[SPARK-21588][SQL] SQLContext.getConf(key, null) should return null #18852

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.

Sign up for GitHub

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

Closed
vinodkc wants to merge 2 commits into from
Closed

[SPARK-21588][SQL] SQLContext.getConf(key, null) should return null #18852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4055e1
SQLContext.getConf(key, null) should return null
vinodkc Aug 5, 2017
4295cd3
testcase updated
vinodkc Aug 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1228,10 +1228,12 @@ class SQLConf extends Serializable with Logging {
* not set yet, return `defaultValue`.
*/
def getConfString(key: String, defaultValue: String): String = {
val entry = sqlConfEntries.get(key)
if (entry != null && defaultValue != "<undefined>") {
// Only verify configs in the SQLConf object
entry.valueConverter(defaultValue)
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)
}
Expand Down
11 changes: 11 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

@viirya viirya Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the key doesn't exist, this doesn't actually test the issue. This line passes without this change too.

Copy link
Member

@viirya viirya Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NPE is only happened for an existing entry like the above SHUFFLE_PARTITIONS or the previous spark.sql.thriftServer.incrementalCollect.

Copy link
Member

@gatorsmile gatorsmile Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to improve the test case coverage.

Copy link
Member

@viirya viirya Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good. But with current test, we don't actually test against the case of NPE which is the main issue.

Copy link
Member

@gatorsmile gatorsmile Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew it, but I just do not want to introduce any regression. Thus, I just to cover both scenarios.

Copy link
Member

@viirya viirya Aug 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, those tests are good. We just should add another test for NPE case too. Otherwise, there's no regression test for the change from this PR.

Copy link
Member

@gatorsmile gatorsmile Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line 276 does not verify the fix? Why we still need to add another test case?

Copy link
Member

@viirya viirya Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, right. Sorry. Miss it.

assert("<undefined>" == spark.conf.get("spark.sql.nonexistent", "<undefined>"))
}
}