[SPARK-26303][SQL] Return partial results for bad JSON records #23253

MaxGekk · 2018-12-07T10:09:10Z

What changes were proposed in this pull request?

In the PR, I propose to return partial results from JSON datasource and JSON functions in the PERMISSIVE mode if some of JSON fields are parsed and converted to desired types successfully. The changes are made only for StructType. Whole bad JSON records are placed into the corrupt column specified by the columnNameOfCorruptRecord option or SQL config.

Partial results are not returned for malformed JSON input.

How was this patch tested?

Added new UT which checks converting JSON strings with one invalid and one valid field at the end of the string.

This reverts commit 542ae74.

SparkQA · 2018-12-07T11:42:43Z

Test build #99824 has finished for PR 23253 at commit e13673d.

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

MaxGekk · 2018-12-07T12:14:45Z

jenkins, retest this, please

viirya · 2018-12-07T15:30:35Z

docs/sql-migration-guide-upgrade.md

@@ -37,6 +37,8 @@ displayTitle: Spark SQL Upgrading Guide

  - In Spark version 2.4 and earlier, CSV datasource converts a malformed CSV string to a row with all `null`s in the PERMISSIVE mode. Since Spark 3.0, returned row can contain non-`null` fields if some of CSV column values were parsed and converted to desired types successfully.

+  - In Spark version 2.4 and earlier, JSON datasource and JSON functions like `from_json` convert a bad JSON record to a row with all `null`s in the PERMISSIVE mode when specified schema is `StructType`. Since Spark 3.0, returned row can contain non-`null` fields if some of JSON column values were parsed and converted to desired types successfully.
+


hmm, if returned row contains non null fields, how do we know if the row is read from a bad JSON record or a correct JSON record?

And this behavior is also defined at some places like DataFrameReader.

If the row was produced from a bad JSON record, the bad record is placed to the corrupt column otherwise the corrupt column contains null.

If there is no corrupt column?

In the PERMISSIVE mode, no way but at the moment (without the PR) you cannot distinguish a row produced from a bad record from a row produced from JSON object with all null fields too.

A row itself with all null cannot be an indicator of bad record. Need an additional flag. null or non-null in the corrupt column plays such role.

For such behavior change, shall we add a config to roll back to previous behavior?

And you should also update other places where defines previous behavior, like DataFrameReader.

@cloud-fan This PR propose similar changes as in #23120 . Could you take a look at it.

For such behavior change, shall we add a config to roll back to previous behavior?

I don't think it makes sense to introduce global SQL config for this particular case. The risk of breaking users apps is low because apps logic cannot based only on presence of all nulls in row. All nulls don't differentiate bad and not-bad JSON records. From my point of view, a note in the migration guide is enough.

Ok. Sounds reasonable.

SparkQA · 2018-12-07T16:02:00Z

Test build #99828 has finished for PR 23253 at commit e13673d.

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

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonSuite.scala

SparkQA · 2018-12-08T14:36:18Z

Test build #99874 has finished for PR 23253 at commit 62a6795.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-12-08T18:40:09Z

Test build #99875 has finished for PR 23253 at commit b19b3e1.

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

docs/sql-migration-guide-upgrade.md

SparkQA · 2018-12-08T23:33:35Z

Test build #99879 has finished for PR 23253 at commit fc088df.

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

dongjoon-hyun · 2018-12-09T01:50:56Z

cc @HyukjinKwon

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/TestJsonData.scala

cloud-fan · 2018-12-09T07:13:13Z

docs/sql-migration-guide-upgrade.md

-  - In Spark version 2.4 and earlier, CSV datasource converts a malformed CSV string to a row with all `null`s in the PERMISSIVE mode. Since Spark 3.0, returned row can contain non-`null` fields if some of CSV column values were parsed and converted to desired types successfully.
+  - In Spark version 2.4 and earlier, CSV datasource converts a malformed CSV string to a row with all `null`s in the PERMISSIVE mode. Since Spark 3.0, the returned row can contain non-`null` fields if some of CSV column values were parsed and converted to desired types successfully.
+
+  - In Spark version 2.4 and earlier, JSON datasource and JSON functions like `from_json` convert a bad JSON record to a row with all `null`s in the PERMISSIVE mode when specified schema is `StructType`. Since Spark 3.0, the returned row can contain non-`null` fields if some of JSON column values were parsed and converted to desired types successfully.


does from_csv support it?

from_csv was added recently. It didn't exist in 2.4

cloud-fan · 2018-12-09T13:16:24Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala

+    if (badRecordException.isEmpty) {
+      row
+    } else {
+      throw BadRecordException(() => UTF8String.EMPTY_UTF8, () => Some(row), badRecordException.get)


add a comment to say that, we don't know the original record here, and it will be filled later.

or we can create a new exception type and use it here, which only carries the row and the exception

I added a separate exception

cloud-fan · 2018-12-09T13:18:20Z

LGTM except a code style comment

SparkQA · 2018-12-09T13:57:14Z

Test build #99887 has finished for PR 23253 at commit 07e0498.

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

dongjoon-hyun · 2018-12-09T19:01:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/BadRecordException.scala

+ * @param partialResult the partial result of parsing a bad record.
+ * @param cause the actual exception about why the parser cannot return full result.
+ */
+case class PartialResultException(


Ur, is this intentional? It looks like javax.naming.PartialResultException to me. Not only the same name, but also the semantics.

@cloud-fan . Is this okay? Or, shall we use more distinguishable name like SparkPartialResultException instead?

We need to carry the InternalRow here. I'm fine with the current name, as we don't have Spark prefix in BadRecordException

I got it~ Thanks.

Wait .. but let's just rename it if possible .. the cost of renaming is 0 but there are some benefits by that ..

I mean, we don't have to standardise the name but let's use another name that doesn't conflict with Java's libraries.

SparkQA · 2018-12-09T19:13:37Z

Test build #99888 has finished for PR 23253 at commit 9ca9248.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PartialResultException(

HyukjinKwon · 2018-12-11T06:03:25Z

retest this please

SparkQA · 2018-12-11T08:05:01Z

Test build #99953 has finished for PR 23253 at commit 9ca9248.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PartialResultException(

HyukjinKwon

LGTM. Let me just get this in. Looks the exception is not going to be exposed to users anyway.

HyukjinKwon · 2018-12-11T08:06:13Z

Merged to master.

## What changes were proposed in this pull request? In the PR, I propose to return partial results from JSON datasource and JSON functions in the PERMISSIVE mode if some of JSON fields are parsed and converted to desired types successfully. The changes are made only for `StructType`. Whole bad JSON records are placed into the corrupt column specified by the `columnNameOfCorruptRecord` option or SQL config. Partial results are not returned for malformed JSON input. ## How was this patch tested? Added new UT which checks converting JSON strings with one invalid and one valid field at the end of the string. Closes apache#23253 from MaxGekk/json-bad-record. Lead-authored-by: Maxim Gekk <max.gekk@gmail.com> Co-authored-by: Maxim Gekk <maxim.gekk@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

MaxGekk and others added 8 commits December 6, 2018 22:26

Test for bad records

2d2fed9

Added a test

de48858

Return partial results

101df4e

Updating the migration guide

542ae74

Revert "Updating the migration guide"

439f57a

This reverts commit 542ae74.

Updating the migration guide - separate notes

77d7670

Updating the migration guide - minor

fa431ee

Updating the migration guide - minor 2

e13673d

viirya reviewed Dec 7, 2018

View reviewed changes

dongjoon-hyun reviewed Dec 7, 2018

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonSuite.scala Outdated Show resolved Hide resolved

MaxGekk added 3 commits December 8, 2018 15:19

Test simplification

2dc77ad

other fields -> malformed fields

4311225

CSV: other fields -> malformed fields

62a6795

Making scalastyle checker happy

b19b3e1

dongjoon-hyun reviewed Dec 8, 2018

View reviewed changes

docs/sql-migration-guide-upgrade.md Outdated Show resolved Hide resolved

Added the

fc088df

dongjoon-hyun approved these changes Dec 9, 2018

View reviewed changes

cloud-fan reviewed Dec 9, 2018

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/TestJsonData.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 9, 2018

View reviewed changes

Moving badRecords to JsonSuite

07e0498

cloud-fan reviewed Dec 9, 2018

View reviewed changes

viirya approved these changes Dec 9, 2018

View reviewed changes

Adding PartialResultException

9ca9248

dongjoon-hyun reviewed Dec 9, 2018

View reviewed changes

HyukjinKwon approved these changes Dec 11, 2018

View reviewed changes

asfgit closed this in 4e1d859 Dec 11, 2018

MaxGekk mentioned this pull request Jan 2, 2019

[SPARK-26376][SQL] Skip inputs without tokens by JSON datasource #23325

Closed

MaxGekk mentioned this pull request Jan 14, 2019

[SPARK-25935][SQL] Allow null rows for bad records from JSON parsers #23543

Closed

MaxGekk deleted the json-bad-record branch August 17, 2019 13:35

MaxGekk mentioned this pull request Oct 4, 2022

[SPARK-40646][SQL] Fix returning partial results in JSON data source and JSON functions #38090

Closed

		@@ -37,6 +37,8 @@ displayTitle: Spark SQL Upgrading Guide

		- In Spark version 2.4 and earlier, CSV datasource converts a malformed CSV string to a row with all `null`s in the PERMISSIVE mode. Since Spark 3.0, returned row can contain non-`null` fields if some of CSV column values were parsed and converted to desired types successfully.

		- In Spark version 2.4 and earlier, JSON datasource and JSON functions like `from_json` convert a bad JSON record to a row with all `null`s in the PERMISSIVE mode when specified schema is `StructType`. Since Spark 3.0, returned row can contain non-`null` fields if some of JSON column values were parsed and converted to desired types successfully.

[SPARK-26303][SQL] Return partial results for bad JSON records #23253

[SPARK-26303][SQL] Return partial results for bad JSON records #23253

Uh oh!

Conversation

MaxGekk commented Dec 7, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Dec 7, 2018

Uh oh!

MaxGekk commented Dec 7, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 7, 2018

Uh oh!

Uh oh!

SparkQA commented Dec 8, 2018

Uh oh!

SparkQA commented Dec 8, 2018

Uh oh!

Uh oh!

SparkQA commented Dec 8, 2018

Uh oh!

dongjoon-hyun commented Dec 9, 2018

Uh oh!

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!

cloud-fan commented Dec 9, 2018

Uh oh!

SparkQA commented Dec 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Dec 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 9, 2018

Uh oh!

HyukjinKwon commented Dec 11, 2018

Uh oh!

SparkQA commented Dec 11, 2018

Uh oh!

HyukjinKwon left a comment • 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 Dec 11, 2018

dongjoon-hyun Dec 11, 2018 •

edited

Loading

HyukjinKwon left a comment •

edited

Loading