Skip to content

[SPARK-26151][SQL] Return partial results for bad CSV records - #23120

Closed
MaxGekk wants to merge 3 commits into
apache:masterfrom
MaxGekk:failuresafe-partial-result
Closed

[SPARK-26151][SQL] Return partial results for bad CSV records#23120
MaxGekk wants to merge 3 commits into
apache:masterfrom
MaxGekk:failuresafe-partial-result

Conversation

@MaxGekk

@MaxGekkMaxGekk commented Nov 22, 2018

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

In the PR, I propose to change behaviour of UnivocityParser and FailureSafeParser, and return all fields that were parsed and converted to expected types successfully instead of just returning a row with all nulls for a bad input in the PERMISSIVE mode. For example, for CSV line 0,2013-111-11 12:13:14 and DDL schema a int, b timestamp, new result is Row(0, null).

How was this patch tested?

It was checked by existing tests from CsvSuite and CsvFunctionsSuite.

@MaxGekk

MaxGekk commented Nov 22, 2018

Copy link
Copy Markdown
MemberAuthor

@HyukjinKwon@cloud-fan Please, take a look at the PR.

@SparkQA

SparkQA commented Nov 23, 2018

Copy link
Copy Markdown

Test build #99201 has finished for PR 23120 at commit 8f2d69d.

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

MaxGekk referenced this pull request Nov 23, 2018
@MaxGekk@HyukjinKwon
[SPARK-26108][SQL] Support custom lineSep in CSV datasource
8e8d117
## What changes were proposed in this pull request?
In the PR, I propose new options for CSV datasource - `lineSep` similar to Text and JSON datasource. The option allows to specify custom line separator of maximum length of 2 characters (because of a restriction in `uniVocity` parser). New option can be used in reading and writing CSV files.
## How was this patch tested?
Added a few tests with custom `lineSep` for enabled/disabled `multiLine` in read as well as tests in write. Also I added roundtrip tests.
Closes#23080 from MaxGekk/csv-line-sep.
Lead-authored-by: Maxim Gekk <max.gekk@gmail.com>
Co-authored-by: Maxim Gekk <maxim.gekk@databricks.com>
Signed-off-by: hyukjinkwon <gurwls223@apache.org>
@MaxGekk

MaxGekk commented Nov 27, 2018

Copy link
Copy Markdown
MemberAuthor

@HyukjinKwon Please, review the PR.

@MaxGekk

MaxGekk commented Nov 28, 2018

Copy link
Copy Markdown
MemberAuthor

@cloud-fan May I ask you to take a look at the PR.

cloud-fan
cloud-fan reviewed Nov 28, 2018
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/UnivocityParser.scala
// we just need to convert the tokens that correspond to the required columns.
var badRecordException: Option[Throwable] = None
var i = 0
while (i < requiredSchema.length) {

@cloud-fancloud-fanNov 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shall we stop parsing when we hit the first exception?

@MaxGekkMaxGekkNov 28, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

but we will lose field values that could be converted successfully after the exception.

@cloud-fancloud-fanNov 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know it's doable for CSV, as the tokens are separated ahead, and we can keep parsing after an exception. Is it also doable for other text based data sources?

@MaxGekkMaxGekkNov 30, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It depends on what kind of error we face to. If a parser is still in normal state and ready to continue, we could skip current error. In case of JSON, we parse input in stream fashion, and convert values to desired type on the fly. If JacksonParser is able to recognize next token, why we should stop on the first error?

cloud-fan
cloud-fan reviewed Nov 28, 2018
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/FailureSafeParser.scala
}
resultRow(corruptFieldIndex.get) = badRecord()
resultRow
(row, badRecord) => {

@cloud-fancloud-fanNov 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

without this change in FailureSafeParser, does JSON support returning partial result?

@MaxGekkMaxGekkNov 28, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

For now JSON does not support this. Need additional changes in JacksonParser to return partial results.

cloud-fan
cloud-fan reviewed Dec 2, 2018
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/UnivocityParser.scala Outdated
@cloud-fan

cloud-fan commented Dec 2, 2018

Copy link
Copy Markdown
Contributor

retest this please

@SparkQA

SparkQA commented Dec 2, 2018

Copy link
Copy Markdown

Test build #99563 has finished for PR 23120 at commit 8f2d69d.

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

@SparkQA

SparkQA commented Dec 2, 2018

Copy link
Copy Markdown

Test build #99572 has finished for PR 23120 at commit e09d417.

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

@cloud-fan

cloud-fan commented Dec 3, 2018

Copy link
Copy Markdown
Contributor

thanks, merging to master!

@asfgitasfgit closed this in 11e5f1bDec 3, 2018
@cloud-fan

cloud-fan commented Dec 5, 2018

Copy link
Copy Markdown
Contributor

Hi @MaxGekk , since this changes the result(although makes it better), do you mind adding a migration guide? thanks!

@MaxGekkMaxGekk mentioned this pull request Dec 5, 2018
Closed
@MaxGekk

MaxGekk commented Dec 5, 2018

Copy link
Copy Markdown
MemberAuthor

The PR #23235 updates the sql migration guide

@HyukjinKwon

HyukjinKwon commented Dec 5, 2018

Copy link
Copy Markdown
Member

a late LGTM as well

asfgit pushed a commit that referenced this pull request Dec 5, 2018
@MaxGekk@cloud-fan
[SPARK-26151][SQL][FOLLOWUP] Return partial results for bad CSV records
dd518a1
## What changes were proposed in this pull request?
Updated SQL migration guide according to changes in #23120Closes#23235 from MaxGekk/failuresafe-partial-result-followup.
Lead-authored-by: Maxim Gekk <maxim.gekk@databricks.com>
Co-authored-by: Maxim Gekk <max.gekk@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@MaxGekkMaxGekk mentioned this pull request Dec 8, 2018
Closed
@MaxGekkMaxGekk mentioned this pull request Jan 2, 2019
Closed
@MaxGekkMaxGekk mentioned this pull request Jan 14, 2019
Closed
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@MaxGekkJackey Lee
[SPARK-26151][SQL] Return partial results for bad CSV records
9227b57
## What changes were proposed in this pull request?
In the PR, I propose to change behaviour of `UnivocityParser` and `FailureSafeParser`, and return all fields that were parsed and converted to expected types successfully instead of just returning a row with all `null`s for a bad input in the `PERMISSIVE` mode. For example, for CSV line `0,2013-111-11 12:13:14` and DDL schema `a int, b timestamp`, new result is `Row(0, null)`.
## How was this patch tested?
It was checked by existing tests from `CsvSuite` and `CsvFunctionsSuite`.
Closesapache#23120 from MaxGekk/failuresafe-partial-result.
Authored-by: Maxim Gekk <max.gekk@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@MaxGekkJackey Lee
[SPARK-26151][SQL][FOLLOWUP] Return partial results for bad CSV records
3e1d7d6
## What changes were proposed in this pull request?
Updated SQL migration guide according to changes in apache#23120Closesapache#23235 from MaxGekk/failuresafe-partial-result-followup.
Lead-authored-by: Maxim Gekk <maxim.gekk@databricks.com>
Co-authored-by: Maxim Gekk <max.gekk@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@MaxGekk
MaxGekk deleted the failuresafe-partial-result branch August 17, 2019 13:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MaxGekk@SparkQA@cloud-fan@HyukjinKwon