Skip to content

[SPARK-23094][SPARK-23723][SPARK-23724][SQL][FOLLOW-UP] Support custom encoding for json files - #21254

Closed
gatorsmile wants to merge 2 commits into
apache:masterfrom
gatorsmile:followupSPARK-23094
Closed

[SPARK-23094][SPARK-23723][SPARK-23724][SQL][FOLLOW-UP] Support custom encoding for json files#21254
gatorsmile wants to merge 2 commits into
apache:masterfrom
gatorsmile:followupSPARK-23094

Conversation

@gatorsmile

@gatorsmilegatorsmile commented May 7, 2018

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This is to add a test case to check the behaviors when users write json in the specified UTF-16/UTF-32 encoding with multiline off.

How was this patch tested?

N/A

@gatorsmile

gatorsmile commented May 7, 2018

Copy link
Copy Markdown
MemberAuthor

cc @MaxGekk@HyukjinKwon Do we have any behavior change after the previous PR: #20937?

@HyukjinKwon

HyukjinKwon commented May 7, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Nope, I am quite sure that we don't have any kind of hidden behaviour change. Both lineSep and encoding options are new. Also, these are actually now restricter than it's actually needed for now. Writing can actually work and #21247 tries to allow it; however, I left a comment for him to just focus on getting rid of the restrictions in both read / write side, which I believe is his final goal in 2.4.0 (or 3.0.0).

HyukjinKwon
HyukjinKwon approved these changes May 7, 2018
View reviewed changes

@HyukjinKwonHyukjinKwon left a comment

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.

adding the test seems ok if you feel in that way. this might have to be removed soon within the next release since we should allow this case anyway.

HyukjinKwon
HyukjinKwon reviewed May 7, 2018
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonSuite.scala
withTempPath { path =>
val ds = spark.createDataset(Seq(
("a", 1), ("b", 2), ("c", 3))
).repartition(2)

@HyukjinKwonHyukjinKwonMay 7, 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.

we don't have to repartition though.

@SparkQA

SparkQA commented May 7, 2018

Copy link
Copy Markdown

Test build #90292 has finished for PR 21254 at commit d4c290e.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@MaxGekk

MaxGekk commented May 7, 2018

Copy link
Copy Markdown
Member

Do we have any behavior change after the previous PR: #20937?

The PR brought the encoding (and charset) option but we didn't change behavior when encoding is not specified.

As @HyukjinKwon wrote above the PR #21247 eliminates restrictions in write but the restrictions don't break previous behavior (before #20937) in any case.

@HyukjinKwon

HyukjinKwon commented May 8, 2018

Copy link
Copy Markdown
Member

retest this please

@SparkQA

SparkQA commented May 8, 2018

Copy link
Copy Markdown

Test build #90347 has finished for PR 21254 at commit d4c290e.

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

@HyukjinKwon

HyukjinKwon commented May 8, 2018

Copy link
Copy Markdown
Member

Merged to master.

@asfgitasfgit closed this in 2f6fe7dMay 8, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwonHyukjinKwonHyukjinKwon approved these changes

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

@gatorsmile@HyukjinKwon@SparkQA@MaxGekk