-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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.
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
[SPARK-38464][CORE] Use error classes in org.apache.spark.io #41277
Conversation
errorClass = "CODEC_NOT_AVAILABLE", | ||
messageParameters = Map( | ||
"codecName" -> codecName, | ||
"configKey" -> configKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, wrap it by toSQLConf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Should we move these to spark-core? Or have them duplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry. Let's define in core
at leats quoteByDefault()
, toConf
, and toConfVal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
messageParameters = Map( | ||
"codecName" -> codecName, | ||
"configKey" -> configKey, | ||
"configVal" -> FALLBACK_COMPRESSION_CODEC))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you wrap it by toSQLConfVal
core/src/test/scala/org/apache/spark/io/CompressionCodecSuite.scala
Outdated
Show resolved
Hide resolved
codec.getOrElse(throw new SparkIllegalArgumentException( | ||
errorClass = "CODEC_NOT_AVAILABLE", | ||
messageParameters = Map( | ||
"codecName" -> codecName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, wrap it, for instance by toSQLValue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toSQLValue
is defined in spark-sql, we cannot use it directly here in spark-core.
+1, LGTM. Merging to master. |
### What changes were proposed in this pull request? This PR aims to change exceptions created in package org.apache.spark.io to use error class. This PR also adds `toConf` and `toConfVal` in `SparkCoreErrors`. ### Why are the changes needed? This is to move exceptions created in package org.apache.spark.io to error class. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Updated existing tests. Closes apache#41277 from bozhang2820/spark-38464. Authored-by: Bo Zhang <bo.zhang@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>
What changes were proposed in this pull request?
This PR aims to change exceptions created in package org.apache.spark.io to use error class.
This PR also adds
toConf
andtoConfVal
inSparkCoreErrors
.Why are the changes needed?
This is to move exceptions created in package org.apache.spark.io to error class.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Updated existing tests.