[SPARK-17916][SQL] Fix empty string being parsed as null when nullValue is set. #20068

aa8y · 2017-12-23T21:35:53Z

What changes were proposed in this pull request?

When the option nullValue is set, the empty value is also set to the same value. Therefore empty strings get parsed as null, which should not happen. This PR explicitly changes this to be an empty string.

How was this patch tested?

Tests were added without the fix. It was tested that they failed. Then the fix was added and the tests have been ensured to pass.

Please review http://spark.apache.org/contributing.html before opening a pull request.

…set.

aa8y · 2017-12-23T21:39:59Z

@gatorsmile I've created this PR since #12904 has not been updated in a while.

HyukjinKwon · 2017-12-24T04:52:55Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVOptions.scala

    writerSettings.setIgnoreTrailingWhitespaces(ignoreTrailingWhiteSpaceFlagInWrite)
    writerSettings.setNullValue(nullValue)
-    writerSettings.setEmptyValue(nullValue)
+    writerSettings.setEmptyValue("")


Can we simply expose this as an option and keep the previous behaviour if this option is not set explicitly by the user?

I disagree. I don't think the previous behavior should not be exposed as an option as the previous behavior was a bug. All it did was that it always coerced empty values to nulls. If the nullValue was not set, then the it was set to "" by default which coerced "" to null. The empty value being set to "" had no affect in this case. If it was set to something else, say \N, then the empty value was also set to \N which resulted in parsing both \N and "" to null, as "" was no longer considered as an empty value and the "" being coerced to null is the Univocity parser's default.

Setting empty value explicitly to the "" literal would ensure that an empty string is always parsed as empty string, unless nullValue is not set or it is set to "", which is what people would do if they want "" to be parsed as null, which would be the old behavior.

Could we leave some comments here and update the PR description too?

HyukjinKwon · 2017-12-24T04:54:44Z

ok to test

SparkQA · 2017-12-24T07:55:19Z

Test build #85353 has finished for PR 20068 at commit ebe2900.

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

HyukjinKwon · 2017-12-25T03:23:12Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

  }
+
+  test("SPARK-17916: An empty string should not be coerced to null when nullValue is passed.") {
+    val sparkSession = spark


I think we can just use spark.

HyukjinKwon · 2017-12-25T03:27:00Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVOptions.scala

    writerSettings.setIgnoreTrailingWhitespaces(ignoreTrailingWhiteSpaceFlagInWrite)
    writerSettings.setNullValue(nullValue)
-    writerSettings.setEmptyValue(nullValue)
+    writerSettings.setEmptyValue("")


Could we leave some comments here and update the PR description too?

HyukjinKwon · 2017-12-25T03:28:38Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+      val outDir = new File(dir, "out").getCanonicalPath
+      val nullValue = "\\N"
+
+      import sparkSession.implicits._


I think we don't need this (by import testImplicits._ above).

HyukjinKwon · 2017-12-25T03:29:05Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+      val nullValue = "\\N"
+
+      import sparkSession.implicits._
+      val dsIn = spark.createDataset(elems)


Seq(("bar"), (""), (null: String)).toDS?

HyukjinKwon · 2017-12-25T03:29:56Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+    val elems = Seq(("bar"), (""), (null: String))
+
+    // Checks for new behavior where an empty string is not coerced to null.
+    withTempDir { dir =>


We could do this withTempPath { path =>

HyukjinKwon · 2017-12-25T03:31:01Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+      val expected = Seq(("bar"), (null: String))
+
+      assert(computed.size === 2)
+      assert(computed.sameElements(expected))


We can use checkAnswer(..: DataFrame, .. : DataFrame)

HyukjinKwon · 2017-12-25T03:34:43Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+        .csv(outDir)
+        .as[(String)]
+      val computed = dsOut.collect.toSeq
+      val expected = Seq(("bar"), (null: String))


I don't think this is quite the expected output? Could we use the examples provided in the JIRA rather than single row ones?

aa8y · 2017-12-25T06:49:43Z

@HyukjinKwon I made code changes based on your suggestions. I also changed the tests to use the data mentioned in the ticket. However, you're right, the tests no longer pass. But that is because the Univocity CsvParser, when it encounters an empty string while parsing the data, replaces it with the nullValue we set (see setNullValue()). And the emptyValue is only effective when the empty string being read has quotes around it (see setEmptyValue()). So I believe, at this point, the issue needs to be fixed in the underlying library being used.

SparkQA · 2017-12-25T08:05:01Z

Test build #85368 has finished for PR 20068 at commit 156d755.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2017-12-26T12:39:15Z

retest this please

SparkQA · 2017-12-26T14:53:20Z

Test build #85404 has finished for PR 20068 at commit 156d755.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-12-30T06:59:05Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVOptions.scala

+    // for Spark too, since `nullValue` defaults to an empty string and has a higher precedence to
+    // setEmptyValue(). But when `nullValue` is set to a different value, that would mean that the
+    // empty string should be parsed not as `null` but as an empty string.
+    writerSettings.setEmptyValue("")


Please make it as a conf like what we did for nullValue?

https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala#L613

I talked about this with Hyukjin Kwon before. I think the previous behavior should not be exposed as an option as the previous behavior was a bug. All it did was that it always coerced empty values to nulls. If the nullValue was not set, then the it was set to "" by default which coerced "" to null. The empty value being set to "" had no affect in this case. If it was set to something else, say \N, then the empty value was also set to \N which resulted in parsing both \N and "" to null, as "" was no longer considered as an empty value and the "" being coerced to null is the Univocity parser's default.

Setting empty value explicitly to the "" literal would ensure that an empty string is always parsed as empty string, unless nullValue is not set or it is set to "", which is what people would do if they want "" to be parsed as null, which would be the old behavior.

This conf is needed for users who want to change the behavior back the previous releases. This also needs to be documented in the migration guides in Spark SQL doc.

HyukjinKwon · 2018-01-03T03:28:10Z

Can we make the tests pass BTW, @aa8y?

aa8y · 2018-01-03T03:41:11Z

I'll work on it in the next week or two. That would involve a PR to the Univocity CSV parser.

HyukjinKwon · 2018-01-28T01:51:21Z

ok to test

HyukjinKwon · 2018-01-28T01:51:28Z

ping @aa8y

SparkQA · 2018-01-28T04:12:20Z

Test build #86739 has finished for PR 20068 at commit 156d755.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-04-20T22:11:53Z

ping @aa8y @HyukjinKwon @MaxGekk @gengliangwang

aa8y · 2018-04-20T22:25:07Z

I apologize I haven't had time to work on this. I can close this for now and reopen it when I have a working fix for it.

…ue is set. I propose to bump version of uniVocity parser up to 2.6.3 where quoted empty strings are replaced by the empty value (passed to `setEmptyValue`) instead of `null` values as in the current version 2.5.9: https://github.com/uniVocity/univocity-parsers/blob/v2.6.3/src/main/java/com/univocity/parsers/csv/CsvParser.java#L125 Empty value for writer is set to `""`. So, empty string in dataframe/dataset is stored as empty quoted string `""`. Empty value for reader is set to empty string (zero size). In this way, saved empty quoted string will be read as just empty string. Please, look at the tests for more details. Here are main changes made in [2.6.0](https://github.com/uniVocity/univocity-parsers/releases/tag/v2.6.0), [2.6.1](https://github.com/uniVocity/univocity-parsers/releases/tag/v2.6.1), [2.6.2](https://github.com/uniVocity/univocity-parsers/releases/tag/v2.6.2), [2.6.3](https://github.com/uniVocity/univocity-parsers/releases/tag/v2.6.3): - CSV parser now parses quoted values ~30% faster - CSV format detection process has option provide a list of possible delimiters, in order of priority ( i.e. settings.detectFormatAutomatically( '-', '.');) - uniVocity/univocity-parsers#214 - Implemented trim quoted values support - uniVocity/univocity-parsers#230 - NullPointer when stopping parser when nothing is parsed - uniVocity/univocity-parsers#219 - Concurrency issue when calling stopParsing() - uniVocity/univocity-parsers#231 Closes apache#20068 Added tests from the PR apache#20068 Author: Maxim Gekk <[email protected]> Closes apache#21273 from MaxGekk/univocity-2.6.

zjffdu and others added 5 commits April 29, 2016 11:58

Rebase with master.

1c3d221

Merge remote-tracking branch 'upstream/master'

b4eddd6

Merge remote-tracking branch 'upstream/master'

f406de9

Merge remote-tracking branch 'upstream/master'

762c144

SPARK-17916: Fix empty string being parsed as null when nullValue is …

ebe2900

…set.

aa8y changed the title ~~SPARK-17916: Fix empty string being parsed as null when nullValue is set.~~ [SPARK-17916][SQL] Fix empty string being parsed as null when nullValue is set. Dec 23, 2017

HyukjinKwon reviewed Dec 24, 2017

View reviewed changes

HyukjinKwon reviewed Dec 25, 2017

View reviewed changes

Incorporate code review suggestions.

156d755

gatorsmile reviewed Dec 30, 2017

View reviewed changes

MaxGekk added a commit to MaxGekk/spark that referenced this pull request May 8, 2018

Getting tests from the PR apache#20068

457a21a

MaxGekk mentioned this pull request May 8, 2018

[SPARK-17916][SQL] Fix empty string being parsed as null when nullValue is set. #21273

Closed

asfgit closed this in 7a2d489 May 14, 2018

[SPARK-17916][SQL] Fix empty string being parsed as null when nullValue is set. #20068

[SPARK-17916][SQL] Fix empty string being parsed as null when nullValue is set. #20068

Uh oh!

Conversation

aa8y commented Dec 23, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

aa8y commented Dec 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Dec 24, 2017

Uh oh!

SparkQA commented Dec 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aa8y commented Dec 25, 2017

Uh oh!

SparkQA commented Dec 25, 2017

Uh oh!

HyukjinKwon commented Dec 26, 2017

Uh oh!

SparkQA commented Dec 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jan 3, 2018

Uh oh!

aa8y commented Jan 3, 2018

Uh oh!

HyukjinKwon commented Jan 28, 2018

Uh oh!

HyukjinKwon commented Jan 28, 2018

Uh oh!

SparkQA commented Jan 28, 2018

Uh oh!

gatorsmile commented Apr 20, 2018

Uh oh!

aa8y commented Apr 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gatorsmile Jan 13, 2018 •

edited

Loading