[SPARK-21588][SQL] SQLContext.getConf(key, null) should return null #18852
Conversation
| @@ -808,6 +808,12 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext { | |||
| Row("1")) | |||
| } | |||
|
|
|||
| test("SPARK-21588 SQLContext.getConf(key, null) should return null") { | |||
There was a problem hiding this comment.
Maybe directly test against SQLConf in SQLConfSuite, instead of via SQLContext?
There was a problem hiding this comment.
Thanks for the review comment.
Moved the test to SQLConfSuite
|
Test build #80283 has finished for PR 18852 at commit
|
vinodkc
force-pushed
the
br_Fix_SPARK-21588
branch
from
53b73ed
to
b4055e1
Compare
|
Test build #80287 has finished for PR 18852 at commit
|
| 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>")) |
There was a problem hiding this comment.
The test cases need to be improved.
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>"))
|
LGTM except a comment in test cases. |
|
LGTM pending Jenkins. |
| } | ||
|
|
||
| assert(spark.conf.getOption("spark.sql.nonexistent").isEmpty) | ||
| assert(null == spark.conf.get("spark.sql.nonexistent", null)) |
There was a problem hiding this comment.
Because the key doesn't exist, this doesn't actually test the issue. This line passes without this change too.
There was a problem hiding this comment.
The NPE is only happened for an existing entry like the above SHUFFLE_PARTITIONS or the previous spark.sql.thriftServer.incrementalCollect.
There was a problem hiding this comment.
This is to improve the test case coverage.
There was a problem hiding this comment.
That's good. But with current test, we don't actually test against the case of NPE which is the main issue.
There was a problem hiding this comment.
I knew it, but I just do not want to introduce any regression. Thus, I just to cover both scenarios.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The line 276 does not verify the fix? Why we still need to add another test case?
|
LGTM |
|
Test build #80292 has finished for PR 18852 at commit
|
## 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. Int 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 Author: vinodkc <vinod.kc.in@gmail.com> Closes #18852 from vinodkc/br_Fix_SPARK-21588. (cherry picked from commit 1ba967b) Signed-off-by: gatorsmile <gatorsmile@gmail.com>
## 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. Int 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 Author: vinodkc <vinod.kc.in@gmail.com> Closes #18852 from vinodkc/br_Fix_SPARK-21588. (cherry picked from commit 1ba967b) Signed-off-by: gatorsmile <gatorsmile@gmail.com>
|
Thanks! Merging to master/2.2/2.1 |
## 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. Int 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 Author: vinodkc <vinod.kc.in@gmail.com> Closes apache#18852 from vinodkc/br_Fix_SPARK-21588. (cherry picked from commit 1ba967b) Signed-off-by: gatorsmile <gatorsmile@gmail.com>
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. Int happens only when conf has a value converter Added null check on defaultValue inside SQLConf.getConfString to avoid calling entry.valueConverter(defaultValue) Added unit test Author: vinodkc <vinod.kc.in@gmail.com> Closes apache#18852 from vinodkc/br_Fix_SPARK-21588. (cherry picked from commit 1ba967b) Signed-off-by: gatorsmile <gatorsmile@gmail.com>
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. Int 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