You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "michaelzhan-db (via GitHub)" <gi...@apache.org> on 2024/05/16 22:29:37 UTC
[PR] [SPARK-43815] Wrap NPE with AnalysisException in csv options [spark]
michaelzhan-db opened a new pull request, #46626:
URL: https://github.com/apache/spark/pull/46626
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
When user sets `locale` to be `null`, a NPE is raised. Instead, replace the NPE with a more understandable user facing error message.
### Why are the changes needed?
Improves usability of csv options.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added tests pass.
### Was this patch authored or co-authored using generative AI tooling?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV, XML, and JSON options [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #46626:
URL: https://github.com/apache/spark/pull/46626#issuecomment-2323563704
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV, XML, and JSON options [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46626:
URL: https://github.com/apache/spark/pull/46626#discussion_r1607447215
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala:
##########
@@ -149,7 +149,13 @@ class CSVOptions(
parameters.getOrElse(DateTimeUtils.TIMEZONE_OPTION, defaultTimeZoneId))
// A language tag in IETF BCP 47 format
- val locale: Locale = parameters.get(LOCALE).map(Locale.forLanguageTag).getOrElse(Locale.US)
+ val locale: Locale = parameters.get(LOCALE)
+ .map {
+ case null =>
Review Comment:
My concern is that, in PySpark we ignore when it's `None` vs here we throw an exception. I really think we should fix the whole thing with defining the behaviour instead of fixing one case alone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV, XML, and JSON options [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46626:
URL: https://github.com/apache/spark/pull/46626#discussion_r1607213012
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JSONOptions.scala:
##########
@@ -107,7 +108,13 @@ class JSONOptions(
val writeNullIfWithDefaultValue = SQLConf.get.jsonWriteNullIfWithDefaultValue
// A language tag in IETF BCP 47 format
- val locale: Locale = parameters.get(LOCALE).map(Locale.forLanguageTag).getOrElse(Locale.US)
+ val locale: Locale = parameters.get(LOCALE)
Review Comment:
Let's reduce the duplicated code in CSV/JSON/XML options
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV options [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46626:
URL: https://github.com/apache/spark/pull/46626#discussion_r1604232182
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala:
##########
@@ -149,7 +149,12 @@ class CSVOptions(
parameters.getOrElse(DateTimeUtils.TIMEZONE_OPTION, defaultTimeZoneId))
// A language tag in IETF BCP 47 format
- val locale: Locale = parameters.get(LOCALE).map(Locale.forLanguageTag).getOrElse(Locale.US)
+ val locale: Locale = try {
+ parameters.get(LOCALE).map(Locale.forLanguageTag).getOrElse(Locale.US)
+ } catch {
+ case _: NullPointerException =>
Review Comment:
Can we check if the value is `null`, and throw an exception instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV, XML, and JSON options [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #46626: [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV, XML, and JSON options
URL: https://github.com/apache/spark/pull/46626
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV, XML, and JSON options [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46626:
URL: https://github.com/apache/spark/pull/46626#discussion_r1612625851
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala:
##########
@@ -149,7 +149,13 @@ class CSVOptions(
parameters.getOrElse(DateTimeUtils.TIMEZONE_OPTION, defaultTimeZoneId))
// A language tag in IETF BCP 47 format
- val locale: Locale = parameters.get(LOCALE).map(Locale.forLanguageTag).getOrElse(Locale.US)
+ val locale: Locale = parameters.get(LOCALE)
+ .map {
+ case null =>
Review Comment:
Yeah, that's the key. This is actually a bigger problem than this specific case. I think we should define the behaviour, document somewhere, and apply them for all...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV, XML, and JSON options [spark]
Posted by "michaelzhan-db (via GitHub)" <gi...@apache.org>.
michaelzhan-db commented on code in PR #46626:
URL: https://github.com/apache/spark/pull/46626#discussion_r1612486829
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala:
##########
@@ -149,7 +149,13 @@ class CSVOptions(
parameters.getOrElse(DateTimeUtils.TIMEZONE_OPTION, defaultTimeZoneId))
// A language tag in IETF BCP 47 format
- val locale: Locale = parameters.get(LOCALE).map(Locale.forLanguageTag).getOrElse(Locale.US)
+ val locale: Locale = parameters.get(LOCALE)
+ .map {
+ case null =>
Review Comment:
I see. Pyspark also seems to skip `None` check with `csv()` in python but not when `option()` or `options()` is called.
Would it make sense to just add null checks in scala and `option()`/`options()` in pyspark?
However, I'm not sure if any options accept `null` or `None` as a legitimate value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV, XML, and JSON options [spark]
Posted by "michaelzhan-db (via GitHub)" <gi...@apache.org>.
michaelzhan-db commented on code in PR #46626:
URL: https://github.com/apache/spark/pull/46626#discussion_r1612486829
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala:
##########
@@ -149,7 +149,13 @@ class CSVOptions(
parameters.getOrElse(DateTimeUtils.TIMEZONE_OPTION, defaultTimeZoneId))
// A language tag in IETF BCP 47 format
- val locale: Locale = parameters.get(LOCALE).map(Locale.forLanguageTag).getOrElse(Locale.US)
+ val locale: Locale = parameters.get(LOCALE)
+ .map {
+ case null =>
Review Comment:
I see. Pyspark also seems to skip `None` check with `option()` or `options()` in python.
Would it make sense to just add null checks in scala and `option()`/`options()` in pyspark?
However, I'm not sure if any options accept `null` or `None` as a legitimate value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV, XML, and JSON options [spark]
Posted by "michaelzhan-db (via GitHub)" <gi...@apache.org>.
michaelzhan-db commented on code in PR #46626:
URL: https://github.com/apache/spark/pull/46626#discussion_r1612486829
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala:
##########
@@ -149,7 +149,13 @@ class CSVOptions(
parameters.getOrElse(DateTimeUtils.TIMEZONE_OPTION, defaultTimeZoneId))
// A language tag in IETF BCP 47 format
- val locale: Locale = parameters.get(LOCALE).map(Locale.forLanguageTag).getOrElse(Locale.US)
+ val locale: Locale = parameters.get(LOCALE)
+ .map {
+ case null =>
Review Comment:
I see. Would it make more sense to just add a null check here then?https://github.com/apache/spark/blob/2516fd8439df42b1c161fbd346a0c346cc075f0f/sql/core/src/main/scala/org/apache/spark/sql/DataFrameReader.scala#L108
However, I'm not sure if it would adversely affect other options.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43815][SQL] Wrap NPE with AnalysisException in CSV options [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46626:
URL: https://github.com/apache/spark/pull/46626#discussion_r1604232424
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala:
##########
@@ -149,7 +149,12 @@ class CSVOptions(
parameters.getOrElse(DateTimeUtils.TIMEZONE_OPTION, defaultTimeZoneId))
// A language tag in IETF BCP 47 format
- val locale: Locale = parameters.get(LOCALE).map(Locale.forLanguageTag).getOrElse(Locale.US)
+ val locale: Locale = try {
+ parameters.get(LOCALE).map(Locale.forLanguageTag).getOrElse(Locale.US)
+ } catch {
+ case _: NullPointerException =>
Review Comment:
and this issue should exist in almost all other options. Would probably be best to fix them together
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org