You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "grundprinzip (via GitHub)" <gi...@apache.org> on 2024/05/07 10:22:20 UTC
[PR] [SPARK-41794] Add `try_remainder` function and re-enable column tests [spark]
grundprinzip opened a new pull request, #46434:
URL: https://github.com/apache/spark/pull/46434
### 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 the `try_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
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #46434:
URL: https://github.com/apache/spark/pull/46434#issuecomment-2100956904
@grundprinzip thanks for the work!
Let's also mention the new function in https://spark.apache.org/docs/latest/sql-ref-ansi-compliance.html#useful-functions-for-ansi-mode
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46434:
URL: https://github.com/apache/spark/pull/46434#discussion_r1597553871
##########
sql/core/src/test/scala/org/apache/spark/sql/MathFunctionsSuite.scala:
##########
@@ -707,6 +707,11 @@ class MathFunctionsSuite extends QueryTest with SharedSparkSession {
df1.select(try_divide(make_interval(col("year"), col("month")), lit(0))))
}
+ test("try_remainder") {
+ val df = Seq((10, 3), (5, 5), (5, 0)).toDF("birth", "age")
Review Comment:
Done
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46434:
URL: https://github.com/apache/spark/pull/46434#discussion_r1597553930
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala:
##########
@@ -132,6 +132,43 @@ case class TryDivide(left: Expression, right: Expression, replacement: Expressio
}
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(dividend, divisor) - Returns the remainder after `expr1`/`expr2`. " +
+ "`dividend` must be a numeric. `divisor` must be a numeric.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(3, 2);
+ 1
+ > SELECT _FUNC_(2L, 2L);
+ 0
+ > SELECT _FUNC_(3.0, 2.0);
+ 1.0
+ > SELECT _FUNC_(1, 0);
+ NULL
+ """,
+ since = "3.2.0",
Review Comment:
Done
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46434:
URL: https://github.com/apache/spark/pull/46434#discussion_r1597739768
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/streaming/StreamingQueryListenerBus.scala:
##########
@@ -121,8 +121,10 @@ class StreamingQueryListenerBus(sparkSession: SparkSession) extends Logging {
}
} catch {
case e: Exception =>
- 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)
Review Comment:
```suggestion
logWarning("StreamingQueryListenerBus Handler thread received exception, all client" +
" side listeners are removed and handler thread is terminated.", e)
```
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #46434:
URL: https://github.com/apache/spark/pull/46434#issuecomment-2108398956
Thanks for adding the new function! Merging to master.
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46434:
URL: https://github.com/apache/spark/pull/46434#discussion_r1597739810
##########
connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/client/CheckConnectJvmClientCompatibility.scala:
##########
@@ -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"),
Review Comment:
```suggestion
"org.apache.spark.sql.streaming.StreamingQueryListenerBus"
),
```
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46434:
URL: https://github.com/apache/spark/pull/46434#discussion_r1597739657
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala:
##########
@@ -150,7 +187,7 @@ case class TryDivide(left: Expression, right: Expression, replacement: Expressio
> SELECT _FUNC_(interval 2 year, interval 1 year);
1-0
""",
- since = "3.3.0",
+ since = "4.0.0",
Review Comment:
```suggestion
since = "3.3.0",
```
Is this a mistake?
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46434:
URL: https://github.com/apache/spark/pull/46434#discussion_r1597877323
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala:
##########
@@ -150,7 +187,7 @@ case class TryDivide(left: Expression, right: Expression, replacement: Expressio
> SELECT _FUNC_(interval 2 year, interval 1 year);
1-0
""",
- since = "3.3.0",
+ since = "4.0.0",
Review Comment:
Yes.
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #46434:
URL: https://github.com/apache/spark/pull/46434#issuecomment-2100893973
Thank you.
BTW, the recent test failure is relevant? For me, it looks irrelevant. Could you take a look at?
```
[info] *** 1 TEST FAILED ***
[error] Failed: Total 10578, Failed 1, Errors 0, Passed 10577, Ignored 29
[error] Failed tests:
[error] org.apache.spark.sql.execution.python.PythonStreamingDataSourceSuite
[error] (sql / Test / test) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 3020 s (50:20), completed May 8, 2024, 1:48:01 PM
```
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #46434:
URL: https://github.com/apache/spark/pull/46434#issuecomment-2108420085
@grundprinzip Shall we create a new jira for this instead of using https://issues.apache.org/jira/browse/SPARK-41794?
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #46434:
URL: https://github.com/apache/spark/pull/46434#issuecomment-2109157257
add it to python API references in https://github.com/apache/spark/pull/46566
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #46434:
URL: https://github.com/apache/spark/pull/46434#issuecomment-2099962569
yes, done.
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46434:
URL: https://github.com/apache/spark/pull/46434#discussion_r1597878145
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/streaming/StreamingQueryListenerBus.scala:
##########
@@ -121,8 +121,10 @@ class StreamingQueryListenerBus(sparkSession: SparkSession) extends Logging {
}
} catch {
case e: Exception =>
- 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)
Review Comment:
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
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #46434:
URL: https://github.com/apache/spark/pull/46434#issuecomment-2106336680
@gengliangwang @dongjoon-hyun I addressed all comments and added the additional test, PTAL. Thanks!
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang closed pull request #46434: [SPARK-41794][SQL] Add `try_remainder` function and re-enable column tests
URL: https://github.com/apache/spark/pull/46434
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #46434:
URL: https://github.com/apache/spark/pull/46434#issuecomment-2098973037
Could you fix Python linter failure, @grundprinzip ?
```
./python/pyspark/sql/tests/connect/test_connect_column.py:1029:5: F401 'os' imported but unused
import os
^
1 F401 'os' imported but unused
```
--
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-41794] Add `try_remainder` function and re-enable column tests [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #46434:
URL: https://github.com/apache/spark/pull/46434#issuecomment-2097985397
cc @gengliangwang
--
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-41794][SQL] Add `try_remainder` function and re-enable column tests [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46434:
URL: https://github.com/apache/spark/pull/46434#discussion_r1594306390
##########
sql/core/src/test/scala/org/apache/spark/sql/MathFunctionsSuite.scala:
##########
@@ -707,6 +707,11 @@ class MathFunctionsSuite extends QueryTest with SharedSparkSession {
df1.select(try_divide(make_interval(col("year"), col("month")), lit(0))))
}
+ test("try_remainder") {
+ val df = Seq((10, 3), (5, 5), (5, 0)).toDF("birth", "age")
Review Comment:
Let's have some end-to-end test with decimal input as well, either in this test suite or in `sql/core/src/test/resources/sql-tests/inputs/try_arithmetic.sql`
context: there was a bug found recently https://github.com/apache/spark/pull/46286
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala:
##########
@@ -132,6 +132,43 @@ case class TryDivide(left: Expression, right: Expression, replacement: Expressio
}
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(dividend, divisor) - Returns the remainder after `expr1`/`expr2`. " +
+ "`dividend` must be a numeric. `divisor` must be a numeric.",
+ examples = """
+ Examples:
+ > SELECT _FUNC_(3, 2);
+ 1
+ > SELECT _FUNC_(2L, 2L);
+ 0
+ > SELECT _FUNC_(3.0, 2.0);
+ 1.0
+ > SELECT _FUNC_(1, 0);
+ NULL
+ """,
+ since = "3.2.0",
Review Comment:
```suggestion
since = "4.0.0",
```
--
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