feat: extract TimeType to int/decimal#4997
Conversation
|
Hi @parthchandra, could you help me review this? Thanks! cc @andygrove |
andygrove
left a comment
There was a problem hiding this comment.
Thanks for this, the PR is clean and well tested. I read the four DateTimeUtils methods and the timeExpressions.scala expression classes in Spark to check the behavior, and the acceleration path matches Spark closely. A few notes below, mostly one scope question and a couple of small suggestions.
Integral extraction looks correct. The three integral parts map cleanly to nanosToLocalTime(nanos).getHour/getMinute/getSecond, and routing them through the existing date_part kernels with timezone = "UTC" is right. The nice detail is timezone independence: for a Time64(Nanosecond) input, array_with_timezone falls through its final _ => Ok(array) arm, so the array passes through untouched and arrow's date_part reads the time-of-day fields directly. The unit test checking hour/minute/second across UTC, LA, and Tokyo is a good guard for exactly that.
The fractional-second math matches Spark's truncation. Spark's getSecondsOfTimeWithFraction does (nanos % NANOS_PER_SECOND) * scaleFactor / NANOS_PER_SECOND with integer division, so it truncates. The native second_with_fraction truncates the same way with pure i128 arithmetic. Using integers instead of Spark's Double intermediate is arguably more robust, and at scale 6 the results are identical. The unit test at precisions 0, 3, 6 confirms it.
Main thing worth confirming: the fractional path only supports TIME(6). Spark's SecondsOfTimeWithFraction declares its return type as DecimalType(2 + p, p) where p is the child TimeType precision, and TimeType allows p in [0, 9] (MAX_PRECISION = NANOS_PRECISION = 9). The shim guard is s.dataType == DecimalType(8, 6), which only admits p = 6, and the native function reinforces that by always emitting Decimal128(8, 6). So extract(SECOND FROM t) over a TIME(3) or TIME(9) column will not match the shim and falls back to Spark. That is safe and correct, just not accelerated. It happens to cover the common case because make_time always produces TIME(MICROS_PRECISION) = TIME(6), which is why all the tests hit the native path. Is limiting to TIME(6) the intended scope for this PR? If so, a one-line comment on the DecimalType(8, 6) guard explaining the restriction would help, and a follow-up issue to generalize the other precisions would be nice to link. The integral hour/minute/second path already handles every precision, so only the fractional case is limited.
Minor guard inconsistency. isValidTimePrecision bounds the literal by TimeType.MAX_PRECISION (9), but the native second_with_fraction returns an Execution error for any precision above 6 and always writes scale 6. Today this never bites because the s.dataType == DecimalType(8, 6) guard pins precision to 6 before isValidTimePrecision is reached. If that dataType guard is ever relaxed, the two checks would disagree and TIME(7..9) would produce a native error rather than a clean fallback. It might be clearer to bound isValidTimePrecision by MICROS_PRECISION (6) with a comment, so the Scala and Rust limits agree.
Test coverage suggestion. All the SQL cases build on make_time, so every value is TIME(6). It might be worth adding a CAST(make_time(...) AS TIME(3)) (and maybe TIME(9)) case with spark_answer_only to document that lower and higher precisions still return correct results through the fallback path. An integral hour/minute case at a non-6 precision would also exercise the all-precision native path.
No expressions.md change is needed since hour, minute, second, extract, and date_part are already documented and second_with_fraction is an internal kernel rather than a user-facing function.
Overall this looks good. The only real discussion point is whether the TIME(6)-only restriction on fractional seconds is intended, and making that boundary explicit in the code.
Which issue does this PR close?
Closes #4983
Rationale for this change
Support time extraction (from TimeType to Int/Decimal)
extract(HOUR FROM t)HoursOfTimegetHoursOfTimeextract(MINUTE FROM t)MinutesOfTimegetMinutesOfTimeextract(SECOND FROM t)SecondsOfTimegetSecondsOfTimeextract(SECOND FROM t)(with fraction)SecondsOfTimeWithFractiongetSecondsOfTimeWithFractionWhat changes are included in this PR?
How are these changes tested?
Time64(Nanosecond), including multiple precision values.time_extract.sqlcoveringhour,minute,second,extract(SECOND FROM ...), anddate_part('SECOND', ...).