[SPARK-19137][SQL] Fix `withSQLConf` to reset `OptionalConfigEntry` correctly by dongjoon-hyun · Pull Request #16522 · apache/spark

dongjoon-hyun · 2017-01-09T23:40:56Z

What changes were proposed in this pull request?

DataStreamReaderWriterSuite makes test files in source folder like the followings. Interestingly, the root cause is withSQLConf fails to reset OptionalConfigEntry correctly. In other words, it resets the config into Some(undefined).

$ git status
Untracked files:
(use "git add <file>..." to include in what will be committed)
sql/core/%253Cundefined%253E/
sql/core/%3Cundefined%3E/

How was this patch tested?

Manual.

build/sbt "project sql" test
git status

…e run

vanzin · 2017-01-09T23:55:49Z

Looks ok since this is what other tests here do; but I wonder why this case isn't handled in StreamingQueryManager.scala; it seems to either throw an error or create a new temp directory, but not to create a new directory under the CWD as is the case here.

zsxwing · 2017-01-09T23:56:50Z

@dongjoon-hyun The expected behavior is this test should use a temp folder instead. Looks like it gets <undefined> from some place.

dongjoon-hyun · 2017-01-09T23:59:48Z

Oh, I see. Then, I'll look inside the temp folder generation code and fix that.
Thank you for review, @vanzin and @zsxwing .

dongjoon-hyun · 2017-01-10T00:03:14Z

I found the root cause.

dongjoon-hyun · 2017-01-10T00:43:10Z

Hi, @vanzin and @zsxwing .
It was a bug of withSQLConf.
I think this is correct fix, but we need to see the result of whole result because this is test utility issue.

zsxwing · 2017-01-10T00:58:09Z

- val currentValues = keys.map(key => Try(spark.conf.get(key)).toOption)
+ val currentValues = keys.map { key =>
+ if (spark.conf.contains(key)) {
+ Try(spark.conf.get(key)).toOption


Nit: you can just use Some(spark.conf.get(key)). It's better to not hide exceptions from get.

dongjoon-hyun · 2017-01-10T01:02:36Z

Thank you. I updated it.

SparkQA · 2017-01-10T02:01:54Z

Test build #71099 has finished for PR 16522 at commit bddaa64.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-01-10T02:54:05Z

Test build #71106 has finished for PR 16522 at commit efcfb81.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-01-10T03:18:08Z

Test build #71107 has finished for PR 16522 at commit eceffcb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2017-01-10T06:55:10Z

Hi, @vanzin and @zsxwing .
The PR passes the tests. Could you review this PR again?

vanzin · 2017-01-10T17:29:55Z

 protected def withSQLConf(pairs: (String, String)*)(f: => Unit): Unit = {
 val (keys, values) = pairs.unzip
- val currentValues = keys.map(key => Try(spark.conf.get(key)).toOption)
+ val currentValues = keys.map { key =>


I think this can just be keys.flatMap(conf.get). But I'll let Ryan look since he's more involved with this code.

I see. Thank you, @vanzin .
Could you ping him?

You already did.

Oh. You meant @zsxwing . I see.

@vanzinconf.get won't work as it will use defaultValueString when the key doesn't exist.

zsxwing · 2017-01-10T18:49:07Z

LGTM. Merging to master and 2.1.

…orrectly ## What changes were proposed in this pull request? `DataStreamReaderWriterSuite` makes test files in source folder like the followings. Interestingly, the root cause is `withSQLConf` fails to reset `OptionalConfigEntry` correctly. In other words, it resets the config into `Some(undefined)`. ```bash $ git status Untracked files: (use "git add <file>..." to include in what will be committed) sql/core/%253Cundefined%253E/ sql/core/%3Cundefined%3E/ ``` ## How was this patch tested? Manual. ``` build/sbt "project sql" test git status ``` Author: Dongjoon Hyun <dongjoon@apache.org> Closes#16522 from dongjoon-hyun/SPARK-19137. (cherry picked from commit d5b1dc9) Signed-off-by: Shixiong Zhu <shixiong@databricks.com>

dongjoon-hyun · 2017-01-10T19:39:36Z

Thank you, @zsxwing and @vanzin !

…orrectly ## What changes were proposed in this pull request? `DataStreamReaderWriterSuite` makes test files in source folder like the followings. Interestingly, the root cause is `withSQLConf` fails to reset `OptionalConfigEntry` correctly. In other words, it resets the config into `Some(undefined)`. ```bash $ git status Untracked files: (use "git add <file>..." to include in what will be committed) sql/core/%253Cundefined%253E/ sql/core/%3Cundefined%3E/ ``` ## How was this patch tested? Manual. ``` build/sbt "project sql" test git status ``` Author: Dongjoon Hyun <dongjoon@apache.org> Closesapache#16522 from dongjoon-hyun/SPARK-19137.

[SPARK-19137][SQL][SS] Garbage left in source tree after SQL tests ar…
bddaa64
…e run

Fix withSQLConf to reset OptionalConfigEntry correctly.
efcfb81

dongjoon-hyun changed the title ~~[SPARK-19137][SQL][SS] Garbage left in source tree after SQL tests ran~~[SPARK-19137][SQL] Fix withSQLConf to reset OptionalConfigEntry correctlyJan 10, 2017

zsxwing reviewed Jan 10, 2017
View reviewed changes

Address comments.
eceffcb

vanzin reviewed Jan 10, 2017
View reviewed changes

asfgit closed this in d5b1dc9Jan 10, 2017

dongjoon-hyun deleted the SPARK-19137 branch January 7, 2019 07:03

Uh oh!

Conversation

dongjoon-hyun commented Jan 9, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

vanzin commented Jan 9, 2017

Uh oh!

zsxwing commented Jan 9, 2017

Uh oh!

dongjoon-hyun commented Jan 9, 2017

Uh oh!

dongjoon-hyun commented Jan 10, 2017

Uh oh!

dongjoon-hyun commented Jan 10, 2017

Uh oh!

zsxwingJan 10, 2017

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyunJan 10, 2017

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jan 10, 2017

Uh oh!

SparkQA commented Jan 10, 2017

Uh oh!

SparkQA commented Jan 10, 2017

Uh oh!

SparkQA commented Jan 10, 2017

Uh oh!

dongjoon-hyun commented Jan 10, 2017

Uh oh!

vanzinJan 10, 2017

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyunJan 10, 2017

Choose a reason for hiding this comment

Uh oh!

vanzinJan 10, 2017

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyunJan 10, 2017

Choose a reason for hiding this comment

Uh oh!

zsxwingJan 10, 2017

Choose a reason for hiding this comment

Uh oh!

zsxwing commented Jan 10, 2017

Uh oh!

dongjoon-hyun commented Jan 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dongjoon-hyun commented Jan 9, 2017•
edited
Loading