-
Notifications
You must be signed in to change notification settings - Fork 237
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
Support division on ANSI interval types #5134
Conversation
Signed-off-by: Chong Gao <res_life@163.com>
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.
The temporary changes needed to compile on Spark 3.3 can be removed now that #5124 has been merged.
sql-plugin/src/main/330+/scala/com/nvidia/spark/rapids/shims/Spark33XShims.scala
Outdated
Show resolved
Hide resolved
Waiting #5150 to merge, has some conflict on Spark33XShims. |
Waiting #5150 to merge, has some conflict on Spark33XShims. |
build |
Optimized the division code by leveraging |
sql-plugin/src/main/330+/scala/org/apache/spark/sql/rapids/shims/intervalExpressions.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/330+/scala/org/apache/spark/sql/rapids/shims/intervalExpressions.scala
Show resolved
Hide resolved
build |
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
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.
Tests are no longer passing after the recent changes, which implies they were not rerun after the changes were made. Note that since this test is Spark 3.3.0+, it's not normally run as part of premerge (that may soon change once Spark 3.3 finally ships). In the meantime, it's important to run it manually or we'll end up finding it doesn't pass on the nightly 3.3.0 runs after it already has been merged.
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
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.
Minor nit on checking for NaN values using the dedicated predicate method, but otherwise lgtm.
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
tests/src/test/330/scala/com/nvidia/spark/rapids/IntervalDivisionSuite.scala
Outdated
Show resolved
Hide resolved
build |
CI failure is unrelated, appears to be the (transient?) Cloudera dependency issue. Kicking the build again. |
build |
Contributes #4151
Supports
Note: not support decimal type because of CUDF Decimal128 does not handle overflow, like:
Signed-off-by: Chong Gao res_life@163.com