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

MaxGekk · 2018-05-08T21:40:15Z

What changes were proposed in this pull request?

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, 2.6.1, 2.6.2, 2.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( '-', '.');) - Csv format detection enhancement uniVocity/univocity-parsers#214
Implemented trim quoted values support - Trim quoted columns uniVocity/univocity-parsers#230
NullPointer when stopping parser when nothing is parsed - Stop parsing causes null pointer exception uniVocity/univocity-parsers#219
Concurrency issue when calling stopParsing() - Concurrency issue when calling stopParsing() uniVocity/univocity-parsers#231

Closes #20068

How was this patch tested?

Added tests from the PR #20068

SparkQA · 2018-05-08T21:45:39Z

Test build #90387 has finished for PR 21273 at commit 598ba2d.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-05-09T00:43:48Z

Sounds good @MaxGekk BTW mind adding Closes #20068 in the PR description?

maropu · 2018-05-09T02:07:25Z

We need to list what're differences (new features and bugs) between v2.5.9 and v2.6.3 for checking compatibility and others?

SparkQA · 2018-05-09T02:16:31Z

Test build #90388 has finished for PR 21273 at commit db18345.

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

MaxGekk · 2018-05-10T08:00:22Z

@HyukjinKwon @maropu Please, have a look at the PR.

gatorsmile · 2018-05-10T16:08:41Z

cc @gengliangwang

gatorsmile · 2018-05-10T16:09:35Z

CSV parser now parses quoted values ~30% faster

Could we add a micro-benmark suite for this?

MaxGekk · 2018-05-10T20:10:16Z

CSV parser now parses quoted values ~30% faster

Could we add a micro-benmark suite for this?

@gatorsmile In this PR or in a separate one?

gengliangwang · 2018-05-12T12:15:20Z

LGTM, it would be nice to have a micro-benmark suite in this PR.

MaxGekk · 2018-05-12T21:12:16Z

@gengliangwang @gatorsmile I added a benchmark for parsing of quoted values. Parsing time dropped by 28% (look at the commit f3a0072)

SparkQA · 2018-05-13T01:18:17Z

Test build #90543 has finished for PR 21273 at commit f3a0072.

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

HyukjinKwon

LGTM

gengliangwang

LGTM

maropu · 2018-05-14T01:53:48Z

LGTM

HyukjinKwon · 2018-05-14T02:00:41Z

Merged to master.

…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.

koertkuipers · 2018-08-20T03:35:40Z

to summarize my findings from jira:
this breaks any usage without quoting. for example we remove all characters from our values that need to be quoted (delimiters, newlines) so we know we will always write unquoted csv, but now we suddenly find these empty quoted strings in our output. the systems we write to cannot handle these quoted values.

HyukjinKwon · 2018-08-20T03:58:25Z

@koertkuipers, would you mind if I ask provide a reproducer please?

koertkuipers · 2018-08-20T04:31:51Z

@HyukjinKwon see the jira for the example code that reproduces the issue.
let me know if you need anything else. best, koert

koertkuipers · 2018-08-24T02:22:16Z

i would suggest at least that when the quote character is changed that the empty value should change accordingly. an empty value of "" makes no sense if the quote character is not ".

also if we could agree on a quote character that means no quotes at all (so some non-printable character perhaps) then i would suggest to change empty value back to null if that particular quote character is set. because a quoted empty string never makes sense if the user is trying to write out unquoted values only.

HyukjinKwon · 2018-08-24T03:24:58Z

@koertkuipers you wanna make a PR to make it configuration?

koertkuipers · 2018-09-01T19:44:21Z

@HyukjinKwon see #22312

HyukjinKwon · 2018-09-03T02:07:48Z

#22234 was already open. Wouldn't it be able to workaround if it's configurable?

koertkuipers · 2018-09-03T03:07:36Z

it would provide a workaround i think, yes.

gatorsmile · 2018-09-08T19:13:19Z

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("\"\"")


This needs an update in migration guide.

MaxGekk added 3 commits May 8, 2018 23:23

Getting tests from the PR apache#20068

457a21a

Bump versions of uniVocity parser to 2.6.3

cd78b12

Set values of empty strings in read and in write

598ba2d

Bumping versions of uniVocity parser in manifest files

db18345

MaxGekk changed the title ~~[WIP][SPARK-17916][SQL] 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. May 10, 2018

MaxGekk added 2 commits May 12, 2018 23:05

Benchmarks for uniVocity v2.5.9

137a525

Benchmarks for uniVocity v2.6.3

f3a0072

HyukjinKwon approved these changes May 13, 2018

View reviewed changes

gengliangwang approved these changes May 13, 2018

View reviewed changes

asfgit closed this in 7a2d489 May 14, 2018

gatorsmile mentioned this pull request Sep 4, 2018

[SPARK-25241][SQL] Configurable empty values when reading/writing CSV files #22234

Closed

gatorsmile reviewed Sep 8, 2018

View reviewed changes

MaxGekk deleted the univocity-2.6 branch August 17, 2019 13:33

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

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

Uh oh!

Conversation

MaxGekk commented May 8, 2018 • 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!

SparkQA commented May 8, 2018

Uh oh!

HyukjinKwon commented May 9, 2018

Uh oh!

maropu commented May 9, 2018

Uh oh!

SparkQA commented May 9, 2018

Uh oh!

MaxGekk commented May 10, 2018

Uh oh!

gatorsmile commented May 10, 2018

Uh oh!

gatorsmile commented May 10, 2018

Uh oh!

MaxGekk commented May 10, 2018

Uh oh!

gengliangwang commented May 12, 2018

Uh oh!

MaxGekk commented May 12, 2018

Uh oh!

SparkQA commented May 13, 2018

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

gengliangwang left a comment

Choose a reason for hiding this comment

Uh oh!

maropu commented May 14, 2018

Uh oh!

HyukjinKwon commented May 14, 2018

Uh oh!

koertkuipers commented Aug 20, 2018

Uh oh!

HyukjinKwon commented Aug 20, 2018

Uh oh!

koertkuipers commented Aug 20, 2018

Uh oh!

koertkuipers commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Aug 24, 2018

Uh oh!

koertkuipers commented Sep 1, 2018

Uh oh!

HyukjinKwon commented Sep 3, 2018

Uh oh!

koertkuipers commented Sep 3, 2018

Uh oh!

gatorsmile Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

MaxGekk commented May 8, 2018 •

edited

Loading

koertkuipers commented Aug 24, 2018 •

edited

Loading