Skip to content

[SPARK-20980] [SQL] Rename wholeFile to multiLine for both CSV and JSON #18202

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

Jump to bottom
Closed
gatorsmile wants to merge 3 commits into from
Closed

[SPARK-20980] [SQL] Rename wholeFile to multiLine for both CSV and JSON #18202

gatorsmile wants to merge 3 commits into from

Conversation

@gatorsmile
Copy link
Member

@gatorsmile gatorsmile commented Jun 5, 2017

What changes were proposed in this pull request?

The current option name wholeFile is misleading for CSV users. Currently, it is not representing a record per file. Actually, one file could have multiple records. Thus, we should rename it. Now, the proposal is multiLine.

How was this patch tested?

N/A

@gatorsmile gatorsmile changed the title [SPARK-20980] [SQL] CSV: rename the option wholeFile to multiLine [SPARK-20980] [SQL] Rename the CSV option wholeFile to multiLine Jun 5, 2017
@gatorsmile
Copy link
Member Author

gatorsmile commented Jun 5, 2017 •
edited
Loading

cc @cloud-fan @HyukjinKwon

@SparkQA
Copy link

SparkQA commented Jun 5, 2017

Test build #77733 has started for PR 18202 at commit 1fae66a.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jun 5, 2017

Retest this please.

dongjoon-hyun
dongjoon-hyun reviewed Jun 5, 2017
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVOptions.scala
parameters.getOrElse("timestampFormat", "yyyy-MM-dd'T'HH:mm:ss.SSSXXX"), timeZone, Locale.US)

val wholeFile = parameters.get("wholeFile").map(_.toBoolean).getOrElse(false)
val multiLine = parameters.get("multiLine").map(_.toBoolean).getOrElse(false)
Copy link
Member

@dongjoon-hyun dongjoon-hyun Jun 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, @gatorsmile .
It seems that we need to change JSONOptions.wholeFile together.
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JSONOptions.scala#L84

Copy link
Member Author

@gatorsmile gatorsmile Jun 5, 2017

Choose a reason for hiding this comment

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

That is different. Each JSON file only can parse at most one record when wholeFile is on.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Jun 5, 2017

Choose a reason for hiding this comment

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

Oh, I see. Thank you!

Copy link
Member Author

@gatorsmile gatorsmile Jun 5, 2017

Choose a reason for hiding this comment

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

After rethinking the issue, we need to rename both CSV and JSON to multiLine and fix the JSON parsing to make them consistent.

@gatorsmile gatorsmile changed the title [SPARK-20980] [SQL] Rename the CSV option wholeFile to multiLine [SPARK-20980] [SQL] Rename wholeFile to multiLine for both CSV and JSON Jun 5, 2017
@SparkQA
Copy link

SparkQA commented Jun 5, 2017

Test build #77746 has finished for PR 18202 at commit 1fae66a.

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

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Jun 5, 2017 •
edited
Loading

I think the change itself looks good as targeted to me (if this is going to be included in 2.2.0 - I just saw https://issues.apache.org/jira/browse/SPARK-20980?focusedCommentId=16037416&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16037416). Looks we just need a decision. Probably, please let me cc @rxin who I believe came up the option name initially.

@rxin
Copy link
Contributor

rxin commented Jun 5, 2017

Wouldn't this break compatibility?

@SparkQA
Copy link

SparkQA commented Jun 5, 2017

Test build #77751 has finished for PR 18202 at commit f70994c.

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

@SparkQA
Copy link

SparkQA commented Jun 5, 2017

Test build #77752 has finished for PR 18202 at commit 5780ef5.

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

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Jun 5, 2017 •
edited
Loading

Both options look added in 2.2 assuming from the JIRAs https://issues.apache.org/jira/browse/SPARK-19610 and https://issues.apache.org/jira/browse/SPARK-18352. If it targets 2.2.0, I guess It wouldn't.

@cloud-fan
Copy link
Contributor

cloud-fan commented Jun 8, 2017

let's hold it until RC4 finishes. If RC4 passes, we need to update this PR to support the old option name, otherwise we can just rename.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Jun 14, 2017

Hi all, it sounds RC4 vote was failed. Should we proceed this one?

@asfgit asfgit closed this in 2051428 Jun 15, 2017
@cloud-fan
Copy link
Contributor

cloud-fan commented Jun 15, 2017

ah lucky :) merging to master/2.2!

asfgit pushed a commit that referenced this pull request Jun 15, 2017
@gatorsmile @cloud-fan
[SPARK-20980][SQL] Rename wholeFile to multiLine for both CSV and…
af4f89c 
… JSON

The current option name `wholeFile` is misleading for CSV users. Currently, it is not representing a record per file. Actually, one file could have multiple records. Thus, we should rename it. Now, the proposal is `multiLine`.

N/A

Author: Xiao Li <[email protected]>

Closes #18202 from gatorsmile/renameCVSOption.

(cherry picked from commit 2051428)
Signed-off-by: Wenchen Fan <[email protected]>
@felixcheung
Copy link
Member

felixcheung commented Jun 15, 2017

oops - found this
https://github.com/apache/spark/blame/ae33abf71b353c638487948b775e966c7127cd46/docs/sql-programming-guide.md#L1001

@felixcheung
Copy link
Member

felixcheung commented Jun 15, 2017

opened #18312

dataknocker pushed a commit to dataknocker/spark that referenced this pull request Jun 16, 2017
@gatorsmile
[SPARK-20980][SQL] Rename wholeFile to multiLine for both CSV and…
fb8bcf1 
… JSON

### What changes were proposed in this pull request?
The current option name `wholeFile` is misleading for CSV users. Currently, it is not representing a record per file. Actually, one file could have multiple records. Thus, we should rename it. Now, the proposal is `multiLine`.

### How was this patch tested?
N/A

Author: Xiao Li <[email protected]>

Closes apache#18202 from gatorsmile/renameCVSOption.
@lqshow lqshow mentioned this pull request Jan 7, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@gatorsmile @SparkQA @dongjoon-hyun @HyukjinKwon @rxin @cloud-fan @felixcheung