-
Notifications
You must be signed in to change notification settings - Fork 221
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
Use ErrorClass to Throw AnalysisException [databricks] #10830
Use ErrorClass to Throw AnalysisException [databricks] #10830
Conversation
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
sql-plugin/src/main/scala/org/apache/spark/sql/hive/rapids/RapidsHiveErrors.scala
Outdated
Show resolved
Hide resolved
@jlowe I think I have addressed all your concerns |
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuDataWritingCommandExec.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
This reverts commit 0f765c9.
...plugin/src/main/scala/org/apache/spark/sql/rapids/GpuInsertIntoHadoopFsRelationCommand.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/execution/TrampolineUtil.scala
Outdated
Show resolved
Hide resolved
...plugin/src/main/spark311/scala/org/apache/spark/sql/rapids/shims/RapidsQueryErrorUtils.scala
Show resolved
Hide resolved
...n/src/main/spark340/scala/org/apache/spark/sql/rapids/shims/RapidsErrorUtilsFor340Plus.scala
Outdated
Show resolved
Hide resolved
I have addressed all your concerns I think PTAL. |
build |
...plugin/src/main/scala/org/apache/spark/sql/rapids/GpuInsertIntoHadoopFsRelationCommand.scala
Outdated
Show resolved
Hide resolved
...n/src/main/scala/org/apache/spark/sql/rapids/catalyst/expressions/GpuRandomExpressions.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/execution/TrampolineUtil.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/spark332db/scala/org/apache/spark/sql/rapids/GpuFileFormatWriter.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/spark400/scala/org/apache/spark/sql/rapids/shims/RapidsErrorUtils.scala
Outdated
Show resolved
Hide resolved
I have addressed your comments I think.
|
* AnalysisException is thrown directly rather than via an error | ||
* utility class (this should be rare). | ||
*/ | ||
def throwRapidsAnalysisException(msg: String) = throw new RapidsAnalysisException(msg) |
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.
I'm not sure why we need a TrampolineUtil method to use one of our own classes. Why not have the class that would use this method simply throw RapidsAnalysisException directly?
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.
We don't need a TrampolineUtil
method, this was a remnant of throwing the AnalysisException.
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.
Let me get rid of that. Please let me know if there are any other concerns
build |
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.
I have verified that all of @jlowe's concerns were addressed here.
LGTM!
In Spark 4.0.0 the constructor of AnalysisException that takes a String parameter has been protected. To incorporate this change, we are introducing a shim class that will throw the respective errorClass
contributes to #9259