-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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-41794][SQL] Add try_remainder
function and re-enable column tests
#46434
Conversation
try_remainder
function and re-enable column teststry_remainder
function and re-enable column tests
Could you fix Python linter failure, @grundprinzip ?
|
yes, done. |
Thank you. BTW, the recent test failure is relevant? For me, it looks irrelevant. Could you take a look at?
|
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala
Outdated
Show resolved
Hide resolved
@grundprinzip thanks for the work! |
@gengliangwang @dongjoon-hyun I addressed all comments and added the additional test, PTAL. Thanks! |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala
Outdated
Show resolved
Hide resolved
logWarning( | ||
"StreamingQueryListenerBus Handler thread received exception, all client" + | ||
" side listeners are removed and handler thread is terminated.", | ||
e) |
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.
logWarning( | |
"StreamingQueryListenerBus Handler thread received exception, all client" + | |
" side listeners are removed and handler thread is terminated.", | |
e) | |
logWarning("StreamingQueryListenerBus Handler thread received exception, all client" + | |
" side listeners are removed and handler thread is terminated.", e) |
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 did the spark connect auto format and it produced these changes. I'm ok with reverting them in the worst case but at the same time your suggestions introduce na manual style adjustment.
LMK
@@ -451,8 +451,7 @@ object CheckConnectJvmClientCompatibility { | |||
"org.apache.spark.sql.streaming.RemoteStreamingQuery$"), | |||
// Skip client side listener specific class | |||
ProblemFilters.exclude[MissingClassProblem]( | |||
"org.apache.spark.sql.streaming.StreamingQueryListenerBus" | |||
), | |||
"org.apache.spark.sql.streaming.StreamingQueryListenerBus"), |
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.
"org.apache.spark.sql.streaming.StreamingQueryListenerBus"), | |
"org.apache.spark.sql.streaming.StreamingQueryListenerBus" | |
), |
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.
Will fix the wrong version typo in two places.
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.
Looks fine but I defer to @gengliangwang who drove this.
Fixing versions. Co-authored-by: Gengliang Wang <gengliang@apache.org> Co-authored-by: Hyukjin Kwon <gurwls223@gmail.com>
Thanks for adding the new function! Merging to master. |
@grundprinzip Shall we create a new jira for this instead of using https://issues.apache.org/jira/browse/SPARK-41794? |
add it to python API references in #46566 |
What changes were proposed in this pull request?
As part of re-enabling the ANSI mode tests for Spark Connect, we discovered that we don't have an equivalent for
try_*
for the remainder of operations. This patch adds thetry_remainder
function in Scala, Python, and Spark Connect and adds the required testing.Why are the changes needed?
ANSI and Spark 4
Does this PR introduce any user-facing change?
Yes, it adds the
try_remainder
function that behaves according to ANSI for division by zero.How was this patch tested?
Added new UT and E2E tests.
Was this patch authored or co-authored using generative AI tooling?
No