Skip to content

[SPARK-23723] New charset option for json datasource #20849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MaxGekk wants to merge 37 commits into from
Closed

[SPARK-23723] New charset option for json datasource #20849

Changes from 1 commit
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
b2e92b4
Test for reading json in UTF-16 with BOM
MaxGekk Feb 11, 2018
cb2f27b
Use user's charset or autodetect it if the charset is not specified
MaxGekk Feb 11, 2018
0d45fd3
Added a type and a comment for charset
MaxGekk Feb 13, 2018
1fb9b32
Replacing the monadic chaining by matching because it is more readable
MaxGekk Feb 13, 2018
c3b04ee
Keeping the old method for backward compatibility
MaxGekk Feb 13, 2018
93d3879
testFile is moved into the test to make more local because it is used…
MaxGekk Feb 13, 2018
15798a1
Adding the charset as third parameter to the text method
MaxGekk Feb 13, 2018
cc05ce9
Removing whitespaces at the end of the line
MaxGekk Feb 13, 2018
74f2026
Fix the comment in javadoc style
MaxGekk Feb 13, 2018
4856b8e
Simplifying of the UTF-16 test
MaxGekk Feb 13, 2018
084f41f
A hint to the exception how to set the charset explicitly
MaxGekk Feb 15, 2018
31cd793
Fix for scala style checks
MaxGekk Feb 15, 2018
6eacd18
Run tests again
MaxGekk Feb 15, 2018
3b4a509
Improving of the exception message
MaxGekk Feb 15, 2018
cd1124e
Appended the original message to the exception
MaxGekk Feb 15, 2018
ebf5390
Multi-line reading of json file in utf-32
MaxGekk Feb 17, 2018
c5b6a35
Autodetect charset of jsons in the multiline mode
MaxGekk Feb 17, 2018
ef5e6c6
Test for reading a json in UTF-16LE in the multiline mode by using us…
MaxGekk Feb 17, 2018
f9b6ad1
Fix test: rename the test file - utf32be -> utf32BE
MaxGekk Feb 18, 2018
3b7714c
Fix code style
MaxGekk Feb 18, 2018
edb9167
Appending the create verb to the method for readability
MaxGekk Feb 18, 2018
5ba2881
Making the createParser as a separate private method
MaxGekk Feb 18, 2018
1509e10
Fix code style
MaxGekk Feb 18, 2018
e3184b3
Checks the charset option is supported
MaxGekk Feb 19, 2018
87d259c
Support charset as a parameter of the json method
MaxGekk Feb 19, 2018
76c1d08
Test for charset different from utf-8
MaxGekk Feb 19, 2018
88395b5
Description of the charset option of the json method
MaxGekk Feb 20, 2018
f2f8ae7
Minor changes in comments: added . at the end of a sentence
MaxGekk Feb 21, 2018
b451a03
Added a test for wrong charset name
MaxGekk Feb 21, 2018
c13c159
Testing that charset in any case is acceptable
MaxGekk Feb 21, 2018
1cb3ac0
Test: user specified wrong (but supported) charset
MaxGekk Feb 21, 2018
108e8e7
Set charset as an option
MaxGekk Feb 25, 2018
0d20cc6
Test: saving to json in UTF-32BE
MaxGekk Feb 23, 2018
54baf9f
Taking user's charset for saved json
MaxGekk Feb 23, 2018
1d50d94
Test: output charset is UTF-8 by default
MaxGekk Feb 23, 2018
bb53798
Changing the readJsonFiles method for readability
MaxGekk Mar 4, 2018
961b482
The test checks that json written by Spark can be read back
MaxGekk Mar 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix the comment in javadoc style
  • Loading branch information
@MaxGekk
MaxGekk committed Mar 17, 2018
commit 74f2026e62389902ab7a4c418aa96a492fa14f6f
6 changes: 4 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JSONOptions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,10 @@ private[sql] class JSONOptions(

val multiLine = parameters.get("multiLine").map(_.toBoolean).getOrElse(false)

/** Standard charset name. For example UTF-8, UTF-16 and UTF-32.
* If charset is not specified (None), it will be detected automatically. */
/**
* Standard charset name. For example UTF-8, UTF-16 and UTF-32.
* If charset is not specified (None), it will be detected automatically.

@HyukjinKwon HyukjinKwon Mar 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this detect the encoding for newlines too?

@MaxGekk MaxGekk Mar 18, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you mean the encoding of records/lines delimiter? It depends on the mode. In multiline mode, jackson is able to do that. In the case of per-line mode, Hadoop LinerRecordReader could accept delimiters in any charsets but by defaults it splits input by '\r', '\n', and '\r\n' in UTF-8. This will be fixed in separate PRs for the issues: https://issues.apache.org/jira/browse/SPARK-23724 and https://issues.apache.org/jira/browse/SPARK-23725

@HyukjinKwon HyukjinKwon Mar 18, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shall we fix that first with text datasource since schema inference in JSON is dependent on Text datasource? You are exposing incomplete option now.

@MaxGekk MaxGekk Mar 18, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

schema inference in JSON is dependent on Text datasource

Could you clarify this, please. It is not completely clear to me what do you mean.

@HyukjinKwon HyukjinKwon Mar 18, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See this 8fb2a02. It uses Text datasource when it loads lines when we infer schema.

If we fix encodings with newline first, it's required to Text datasource first I believe.

@HyukjinKwon HyukjinKwon Mar 18, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Json's schema inference use the text datasource to separate the lines before we go through jackson parser where the charset for newlines should be respected. Shouldn't we better fix text datasource with the hadoop's line reader first?

@MaxGekk MaxGekk Mar 18, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

A fix in hadoop line reader and this PR solve 2 different problem. Any fix in hadoop line reader will not fix the problem of wrong encoding detection. I don't understand why this PR must depend on a fix in line reader. I would say a custom record separator will solve newline problem too (https://issues.apache.org/jira/browse/SPARK-23724).

Shouldn't we better fix text datasource with the hadoop's line reader first?

Could you tell me how this PR blocks solving the problem in Hadoop's LineReader?

@HyukjinKwon HyukjinKwon Mar 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you tell me how this PR blocks solving the problem in Hadoop's LineReader?

Because the exposed charset option is incomplete here because the encodings are not respected in newlines.
Also, I want to see how we can solve that problem in SPARK-23724 first too. I am actually not quite worried of the whole changes proposed here for now.

Why don't we just fix that problem first if you plan to fix both eventually anyway?

@MaxGekk MaxGekk Mar 19, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I would like to introduce changes step-by-step to eliminate creating of a "patch bomb". Regarding to SPARK-23724, I am going to propose this PR: MaxGekk#1 /cc @hvanhovell

@HyukjinKwon HyukjinKwon Mar 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yea, step-by-step. I am not suggesting to fix all here. I meant SPARK-23724 should be fixed first with text data source.

*/
val charset: Option[String] = parameters.get("charset")

/** Sets config options on a Jackson [[JsonFactory]]. */
Expand Down