Skip to content

[WiP][SPARK-18699] SQL - parsing CSV should return null for certain numeric … #16319

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
wants to merge 1 commit into from
Closed

[WiP][SPARK-18699] SQL - parsing CSV should return null for certain numeric … #16319

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,9 +246,9 @@ private[csv] object CSVTypeCast {
} else {
castType match {
case _: ByteType => datum.toByte
case _: ShortType => datum.toShort
case _: IntegerType => datum.toInt
case _: LongType => datum.toLong
case _: ShortType => Try(datum.toShort).getOrElse(null)
case _: IntegerType => Try(datum.toInt).getOrElse(null)
case _: LongType => Try(datum.toLong).getOrElse(null)
Copy link
Member

@HyukjinKwon HyukjinKwon Dec 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

So, I guess you meant when the actual data is empty string and nullValue is other values such as NA, they should be translated into null when the mode is PERMISSIVE in this case.

Then, this is a exact duplicate of https://issues.apache.org/jira/browse/SPARK-18699. You could resolve the current JIRA as a duplicate and then fix the title of this PR to [SPARK-18699] ... with some changes suggested there.

Copy link
Author

@kubatyszko kubatyszko Dec 17, 2016

Choose a reason for hiding this comment

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

You're right, fixed the title. Thanks.

Copy link
Member

@maropu maropu Dec 17, 2016

Choose a reason for hiding this comment

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

Why do you use Try for each type here? In the JIRA, I proposed another approach (master...maropu:SPARK-18699), and I think the fix is more natural. Anyway, ISTM we do not decide yet it is worth fixing this issue...

Copy link
Author

@kubatyszko kubatyszko Dec 17, 2016

Choose a reason for hiding this comment

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

My patch was created before I even knew about SPARK-18699 (I did have another ticket open, then learned it was a duplicate).
I do agree that your solution may be more generic and intuitive, if others agree - why wasn't it merged into main tree yet ? (it has been over 10 days since your PR).

Copy link
Member

@maropu maropu Dec 19, 2016

Choose a reason for hiding this comment

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

Behavior changes are much arguable, so it basically takes much time to discuss.

case _: FloatType =>
datum match {
case options.nanValue => Float.NaN
Expand Down