-
Notifications
You must be signed in to change notification settings - Fork 235
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
TimeAdd supports DayTimeIntervalType #3507
Conversation
Signed-off-by: Bobby Wang <wbo4958@gmail.com>
build |
case class GpuTimeAdd(start: Expression, | ||
interval: Expression, | ||
timeZoneId: Option[String] = None) | ||
extends GpuTimeMath(start, interval, timeZoneId) { |
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.
nit: It is a little odd to use GpuTimeMath here when we override essentially all of it.
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.
build |
@@ -0,0 +1,36 @@ | |||
/* | |||
* Copyright (c) 2021, NVIDIA CORPORATION. |
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.
this file has . in the name of it change to directories
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.
Just a few nits, but looks good enough to merge either way.
override def left: Expression = start | ||
override def right: Expression = interval | ||
|
||
override def toString: String = s"$left - $right" |
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.
nit: This is probably a bug in GpuTimeAdd for the older code too, but this should be a +
not a -
. This goes for sql too.
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.
Thx @revans2 , I'd like to merge this PR first, and file a following PR for this, since this PR is blocking my following Range window PR.
@wbo4958 my comment wa snot addressed, please put up PR to fix file name main/301until320-all/scala/org/apache/spark/sql/rapids.shims.v2/datetimeExpressions.scala |
Thx Tom, My bad. really sorry for that. I just had a PR for this. |
This PR is to fix #3502 by adding DayTimeIntervalType support for TimeAdd.
Tested date_time_test.py against spark 3.2.0, and pass
BTW, This RP didn't add DayTimeIntervalType support for TimeSub since it has been deleted from Spark 3.1