[SPARK-26151][SQL] Return partial results for bad CSV records #23120
Conversation
|
@HyukjinKwon @cloud-fan Please, take a look at the PR. |
|
Test build #99201 has finished for PR 23120 at commit
|
## 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>
|
@HyukjinKwon Please, review the PR. |
|
@cloud-fan May I ask you to take a look at the PR. |
| // 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) { |
There was a problem hiding this comment.
shall we stop parsing when we hit the first exception?
There was a problem hiding this comment.
but we will lose field values that could be converted successfully after the exception.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| } | ||
| resultRow(corruptFieldIndex.get) = badRecord() | ||
| resultRow | ||
| (row, badRecord) => { |
There was a problem hiding this comment.
without this change in FailureSafeParser, does JSON support returning partial result?
There was a problem hiding this comment.
For now JSON does not support this. Need additional changes in JacksonParser to return partial results.
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/UnivocityParser.scala
Outdated
Show resolved
Hide resolved
|
retest this please |
|
Test build #99563 has finished for PR 23120 at commit
|
|
Test build #99572 has finished for PR 23120 at commit
|
|
thanks, merging to master! |
|
Hi @MaxGekk , since this changes the result(although makes it better), do you mind adding a migration guide? thanks! |
|
The PR #23235 updates the sql migration guide |
|
a late LGTM as well |
## What changes were proposed in this pull request? Updated SQL migration guide according to changes in #23120 Closes #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>
## 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`. Closes apache#23120 from MaxGekk/failuresafe-partial-result. Authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
## What changes were proposed in this pull request? Updated SQL migration guide according to changes in apache#23120 Closes apache#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>
What changes were proposed in this pull request?
In the PR, I propose to change behaviour of
UnivocityParserandFailureSafeParser, and return all fields that were parsed and converted to expected types successfully instead of just returning a row with allnulls for a bad input in thePERMISSIVEmode. For example, for CSV line0,2013-111-11 12:13:14and DDL schemaa int, b timestamp, new result isRow(0, null).How was this patch tested?
It was checked by existing tests from
CsvSuiteandCsvFunctionsSuite.