[SPARK-16548][SQL] Inconsistent error handling in JSON parsing SQL functions by ewasserman · Pull Request #17693 · apache/spark

ewasserman · 2017-04-20T00:06:58Z

What changes were proposed in this pull request?

change to using Jackson's com.fasterxml.jackson.core.JsonFactory

public JsonParser createParser(String content)

How was this patch tested?

existing unit tests

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

HyukjinKwon · 2017-04-20T00:30:37Z

@ewasserman Could you fix the title to point out the JIRA SPARK-16548? It seems SPARK-20314 duplicates that. Also, I guess we need a test case.

In terms of behaviour, I see @marmbrus and @srowen in that JIRA. Please let me cc.

HyukjinKwon · 2017-04-20T00:40:24Z

 if (parsed.isDefined) {
 try {
- Utils.tryWithResource(jsonFactory.createParser(jsonStr.getBytes)) { parser =>
+ Utils.tryWithResource(jsonFactory.createParser(jsonStr.toString)) { parser =>


I think we should catch the exception below rather than introducing unnecessary string instance apparently. (Sorry, I left a comment and removed back. I thought it was from_json/to_json).

Could you check whether the other exceptions could be thrown by the illegal outputs?

Keep the jsonStr.getBytes unchanged.
/** * Method for constructing parser for parsing * the contents of given byte array. * * @since 2.1 */publicJsonParsercreateParser(byte[] data) throwsIOException, JsonParseException { IOContextctxt = _createContext(data, true); if (_inputDecorator != null) { InputStreamin = _inputDecorator.decorate(ctxt, data, 0, data.length); if (in != null) { return_createParser(in, ctxt); } } return_createParser(data, 0, data.length, ctxt); }
I think we should capture both IOException and JsonParseException.

I thought JsonParseException extends IOException.

gatorsmile · 2017-04-20T06:52:46Z

Could you add a test case in JsonExpressionsSuite?

liancheng · 2017-04-20T22:40:37Z

Not suggesting doing it in this PR but maybe adding a SQL option to let the users choose the error handling strategy of all the JSON functions probably makes more sense here? The Spark JSON data source allows users to choose a parsing mode among:

PERMISSIVE: replacing malformed records with nulls,
DROPMALFORMED: drop malformed records, and
FAILFAST: report an error and abort once a malformed record is found.

gatorsmile · 2017-04-20T23:43:36Z

@liancheng Good suggestion! Just like what we did for from_json/to_json.

HyukjinKwon · 2017-04-20T23:53:08Z

I like the idea but I am not sure of DROPMALFORMED mode though. If we use an expression with the mode enabled, whole record (not only the column but all columns) will be dropped in some json expressions, probably not a generator expressions (did I understand correctly?).

I think we don't explicitly support parse modes in both from_json/to_json -

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala

Line 551 in 4658183

newJSONOptions(options + ("mode"->FailFastMode.name), timeZoneId.get))

It sets FAILFAST but resembles PERMISSIVE mode up to my knowledge.

cloud-fan · 2017-04-21T00:26:57Z


 try {
- Utils.tryWithResource(jsonFactory.createParser(json.getBytes)) {
+ Utils.tryWithResource(jsonFactory.createParser(json.toString)) {


this change has performance penalty, we should just catch more exception below.

ewasserman · 2017-04-21T21:37:53Z

Reverted from use of toString on the org.apache.spark.unsafe.types.UTF8String by running the byte array through a java.io.Reader. This still fixes the bug and is also more efficient on the JSON parser side so it is a net performance win as well.

cloud-fan · 2017-04-22T03:43:29Z

 if (parsed.isDefined) {
 try {
- Utils.tryWithResource(jsonFactory.createParser(jsonStr.getBytes)) { parser =>
+ Utils.tryWithResource(jsonFactory.createParser(new InputStreamReader(


please add some comments to say that, this is to avoid a bug in encoding detection, and we explicitly specify the encoding(UTF8) here.

cloud-fan · 2017-04-22T03:43:36Z

LGTM

gatorsmile · 2017-04-22T16:59:19Z

ok to test

SparkQA · 2017-04-22T19:20:09Z

Test build #76068 has finished for PR 17693 at commit f706ce3.

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

SparkQA · 2017-04-26T01:54:19Z

Test build #76153 has finished for PR 17693 at commit 91bb487.

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

…nctions ## What changes were proposed in this pull request? change to using Jackson's `com.fasterxml.jackson.core.JsonFactory` public JsonParser createParser(String content) ## How was this patch tested? existing unit tests Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Eric Wasserman <ericw@sgn.com> Closes#17693 from ewasserman/SPARK-20314. (cherry picked from commit 57e1da3) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2017-04-26T03:43:42Z

thanks, merging to master/2.2

…nToStructs ## What changes were proposed in this pull request? A fix for the same problem was made in #17693 but ignored `JsonToStructs`. This PR uses the same fix for `JsonToStructs`. ## How was this patch tested? Regression test Author: Burak Yavuz <brkyvz@gmail.com> Closes#17826 from brkyvz/SPARK-20549. (cherry picked from commit 86174ea) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…nToStructs ## What changes were proposed in this pull request? A fix for the same problem was made in apache#17693 but ignored `JsonToStructs`. This PR uses the same fix for `JsonToStructs`. ## How was this patch tested? Regression test Author: Burak Yavuz <brkyvz@gmail.com> Closesapache#17826 from brkyvz/SPARK-20549.

…nctions ## What changes were proposed in this pull request? change to using Jackson's `com.fasterxml.jackson.core.JsonFactory` public JsonParser createParser(String content) ## How was this patch tested? existing unit tests Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Eric Wasserman <ericw@sgn.com> Closesapache#17693 from ewasserman/SPARK-20314. (cherry picked from commit 57e1da3) Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit c8803c0)

fix SPARK-20314
8ef9b6a

HyukjinKwon reviewed Apr 20, 2017
View reviewed changes

cloud-fan reviewed Apr 21, 2017
View reviewed changes

ewasserman changed the title ~~[SPARK-20314][SQL] Inconsistent error handling in JSON parsing SQL functions~~[SPARK-16548][SQL] Inconsistent error handling in JSON parsing SQL functionsApr 21, 2017

more efficient fix; add test
f706ce3

cloud-fan reviewed Apr 22, 2017
View reviewed changes

add requested comment
91bb487

asfgit closed this in 57e1da3Apr 26, 2017

brkyvz mentioned this pull request May 1, 2017
[SPARK-20549] 'java.io.CharConversionException: Invalid UTF-32' in JsonToStructs #17826
Closed

sandip-db mentioned this pull request Jul 12, 2024
[SPARK-48148][CORE] JSON objects should not be modified when read as STRING #46408
Closed

Uh oh!

Conversation

ewasserman commented Apr 20, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon commented Apr 20, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwonApr 20, 2017• 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!

gatorsmileApr 20, 2017

Choose a reason for hiding this comment

Uh oh!

gatorsmileApr 20, 2017

Choose a reason for hiding this comment

Uh oh!

HyukjinKwonApr 20, 2017

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Apr 20, 2017

Uh oh!

liancheng commented Apr 20, 2017

Uh oh!

gatorsmile commented Apr 20, 2017

Uh oh!

HyukjinKwon commented Apr 20, 2017

Uh oh!

cloud-fanApr 21, 2017

Choose a reason for hiding this comment

Uh oh!

ewasserman commented Apr 21, 2017

Uh oh!

cloud-fanApr 22, 2017

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Apr 22, 2017

Uh oh!

gatorsmile commented Apr 22, 2017

Uh oh!

SparkQA commented Apr 22, 2017

Uh oh!

SparkQA commented Apr 26, 2017

Uh oh!

cloud-fan commented Apr 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

HyukjinKwon commented Apr 20, 2017•
edited
Loading

HyukjinKwonApr 20, 2017•
edited
Loading