You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by kubatyszko <gi...@git.apache.org> on 2016/12/16 21:48:08 UTC
[GitHub] spark pull request #16319: SPARK-18906 SQL - parsing CSV should return null ...
GitHub user kubatyszko opened a pull request:
https://github.com/apache/spark/pull/16319
SPARK-18906 SQL - parsing CSV should return null for certain numeric \u2026
## What changes were proposed in this pull request?
CSV parser changes allowing parsing of numeric fields to fail and return null in such case.
In conjunction with "nullValue" option that may be used elsewhere this allows handling of certain csv sources that may use empty string as indication of null in one column and another specific value indicating null in another.
Currently the option "nullValue" can only be provided once and we can't assume that a data source won't have a single "null" indicator.
This problem is very similar to the one discussed here: https://github.com/databricks/spark-csv/issues/239
Sargis Dudaklayan and Kuba Tyszko - Zest Finance
## How was this patch tested?
The patch was tested using freshly compiled spark version 2.0.1 on a sample data source that has "null" values in 2 columns, one specified as "NA" and set using nullValue and another column with "" indicating no integer value.
Please review http://spark.apache.org/contributing.html before opening a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Katlean/spark master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/16319.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #16319
----
commit 0e6b41e01f8c9ac7c8a262bd58f2564a64a638e6
Author: Kuba Tyszko <kt...@zestfinance.com>
Date: 2016-12-16T21:46:43Z
SPARK-18906 SQL - parsing CSV should return null for certain numeric fields - Sargis Dudaklayan and Kuba Tyszko - Zest Finance
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16319: SPARK-18906 SQL - parsing CSV should return null ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/16319#discussion_r92913320
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala ---
@@ -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)
--- End diff --
So, I guess you meant when the actual data is empty string and `nullValue` is other values such as `NA` and in this case, they should be translated into `null` when the `mode` is `PERMISSIVE`.
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 to `[SPARK-18699] ...` with some changes suggested there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16319: [WiP][SPARK-18699] SQL - parsing CSV should retur...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/16319
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16319: [SPARK-18699] SQL - parsing CSV should return nul...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/16319#discussion_r92915632
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala ---
@@ -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)
--- End diff --
Why do you use `Try` for each type here? In the JIRA, I proposed another approach (https://github.com/apache/spark/compare/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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16319: SPARK-18906 SQL - parsing CSV should return null ...
Posted by kubatyszko <gi...@git.apache.org>.
Github user kubatyszko commented on a diff in the pull request:
https://github.com/apache/spark/pull/16319#discussion_r92913976
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala ---
@@ -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)
--- End diff --
You're right, fixed the title. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16319: [SPARK-18699] SQL - parsing CSV should return null for c...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/16319
@HyukjinKwon Thanks for your pinging! I left some comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16319: [SPARK-18699] SQL - parsing CSV should return null for c...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/16319
Let's put `[WIP]` in the PR title, for example, (`[WIP][SPARK-18699] ... `) if you are working on this to follow the suggestion in the JIRA. It seems we need to update the PR description (and with a proper PR title without truncated part such as `...`) and need a test in `CSVSuite`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16319: [WiP][SPARK-18699] SQL - parsing CSV should retur...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/16319#discussion_r92959056
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala ---
@@ -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)
--- End diff --
Behavior changes are much arguable, so it basically takes much time to discuss.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16319: SPARK-18906 SQL - parsing CSV should return null for cer...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/16319
Can you describe which case this PR fixes? I think you meant https://issues.apache.org/jira/browse/SPARK-18699
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16319: SPARK-18906 SQL - parsing CSV should return null for cer...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16319
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16319: [WiP][SPARK-18699] SQL - parsing CSV should return null ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/16319
Hi @kubatyszko, are you still working on this? If you are currently unable to proceed further, maybe it should be closed for now. It seems inactive for few months.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16319: [SPARK-18699] SQL - parsing CSV should return nul...
Posted by kubatyszko <gi...@git.apache.org>.
Github user kubatyszko commented on a diff in the pull request:
https://github.com/apache/spark/pull/16319#discussion_r92916668
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala ---
@@ -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)
--- End diff --
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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org