Add in support for DateAddInterval #1841
Conversation
Signed-off-by: Niranjan Artal <nartal@nvidia.com>
| willNotWorkOnGpu("interval months isn't supported") | ||
| } | ||
| } | ||
| if (ZoneId.of(a.timeZoneId.get).normalized() != GpuOverrides.UTC_TIMEZONE_ID) { |
There was a problem hiding this comment.
typically it's an anti-pattern to call get on an option, especially unconditionally that trips on Nones.
we can do:
a.timeZoneId.foreach {
case zoneId if ZoneId.of(zoneId).normalized() != GpuOverrides.UTC_TIMEZONE_ID =>
willNotWorkOnGpu(s"Only UTC zone id is supported. Actual zone id: $zoneId")
}
There was a problem hiding this comment.
I think it will be simpler to read
if (a.timeZoneId.nonEmpty && ZoneId.of(a.timeZoneId.get).normalized() != GpuOverrides.UTC_TIMEZONE_ID) {
...
}
I am also not sure what the performance impact will be but its not a big deal as its not huge
There was a problem hiding this comment.
Thanks @razajafri . I have incorporated @gerashegalov suggestion here. Let me know if it's okay.
There was a problem hiding this comment.
Can you also fix it in TimeAdd and other places where we might be doing this?
There was a problem hiding this comment.
Sure, Do you want me to fix it in this PR or as a follow on ? I can put up another PR with just those fixes for other expressions.
| "as an argument only") | ||
| } | ||
| } finally { | ||
| if (lhs.isInstanceOf[AutoCloseable]) { |
There was a problem hiding this comment.
Can we use withResourceIfAllowed
| // This is to calculate when subtraction is performed. Need to take into account the | ||
| // interval( which are less than days). Convert it into days which needs to be | ||
| // subtracted along with intvl.days(if provided). | ||
| (intvl.microseconds.abs / (24 * 60 * 60 * 1000 * 1000.0)).ceil.toInt * -1 |
There was a problem hiding this comment.
it would be more readable to convert the numerator .toDouble instead of sneaking in a double as the last factor in the denominator.
Can we compute/define the number of microseconds in a day 24 * 60 * 60 * 1000 * 1000 only once, and reuse the constant. Instead of using our own let us use TimeUnit.DAYS.toMicros(1)
| willNotWorkOnGpu("interval months isn't supported") | ||
| } | ||
| } | ||
| if (ZoneId.of(a.timeZoneId.get).normalized() != GpuOverrides.UTC_TIMEZONE_ID) { |
There was a problem hiding this comment.
I think it will be simpler to read
if (a.timeZoneId.nonEmpty && ZoneId.of(a.timeZoneId.get).normalized() != GpuOverrides.UTC_TIMEZONE_ID) {
...
}
I am also not sure what the performance impact will be but its not a big deal as its not huge
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/datetimeExpressions.scala
Show resolved
Hide resolved
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/datetimeExpressions.scala
Show resolved
Hide resolved
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/datetimeExpressions.scala
Show resolved
Hide resolved
Signed-off-by: Niranjan Artal <nartal@nvidia.com>
|
build |
| } | ||
| val daysToAdd = intvl.days + microSecToDays | ||
| if (daysToAdd != 0) { | ||
| withResource(Scalar.fromInt(daysToAdd)) { us_s => |
There was a problem hiding this comment.
nit: have you tried to use DURATION_DAYS instead, It might avoid all of the casting to INT32. If you cannot do that, then we could use logicalCastTo instead of castTo when going to an INT32 to avoid a small amount of extra data movement. This is fine as is though.
There was a problem hiding this comment.
I hadn't tried DURATION_DAYS earlier but I am getting below exception:
: ai.rapids.cudf.CudfException: NVRTC error: NVRTC_ERROR_COMPILATION
at ai.rapids.cudf.ColumnView.binaryOpVS(Native Method)
at ai.rapids.cudf.ColumnView.binaryOp(ColumnView.java:907)
at ai.rapids.cudf.ColumnView.binaryOp(ColumnView.java:897)
Have updated the code to use logicalCastTo instead of castTo . Please take another look.
There was a problem hiding this comment.
Could you try to clean your cudf jit cache and try again? If you still at least check with cudf if that is expected. I don't think it is and we might need to file a bug with them.
Signed-off-by: Niranjan Artal <nartal@nvidia.com>
|
build |
|
@revans2 Could you please take another look. I think I have addressed the review comment. |
Signed-off-by: Niranjan Artal <nartal@nvidia.com>
Signed-off-by: Niranjan Artal <nartal@nvidia.com>
This fixes #1612
Signed-off-by: Niranjan Artal nartal@nvidia.com