Skip to content

feat: extract TimeType to int/decimal#4997

Open
YutaLin wants to merge 5 commits into
apache:mainfrom
YutaLin:4983_TimeType_to_Int_Decimal
Open

feat: extract TimeType to int/decimal#4997
YutaLin wants to merge 5 commits into
apache:mainfrom
YutaLin:4983_TimeType_to_Int_Decimal

Conversation

@YutaLin

@YutaLin YutaLin commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #4983

Rationale for this change

Support time extraction (from TimeType to Int/Decimal)

SQL Spark expression StaticInvoke method Return type
extract(HOUR FROM t) HoursOfTime getHoursOfTime IntegerType
extract(MINUTE FROM t) MinutesOfTime getMinutesOfTime IntegerType
extract(SECOND FROM t) SecondsOfTime getSecondsOfTime IntegerType
extract(SECOND FROM t) (with fraction) SecondsOfTimeWithFraction getSecondsOfTimeWithFraction DecimalType

What changes are included in this PR?

  • Adds native Comet support for extracting hour, minute, and second from Spark TimeType values.
  • Adds serde handling for Spark’s DateTimeUtils StaticInvoke expressions used by TimeType extraction.
  • Implements fractional-second extraction as Decimal(8, 6) from Arrow Time64(Nanosecond) values.
  • Registers the new second_with_fraction native scalar function.
  • Adds Rust unit tests and Spark SQL coverage for hour, minute, second, extract(SECOND FROM ...), and date_part('SECOND', ...).

How are these changes tested?

  • Added Rust unit tests for extracting hour, minute, integer second, and fractional second from Time64(Nanosecond), including multiple precision values.
  • Added Spark SQL tests in time_extract.sql covering hour, minute, second, extract(SECOND FROM ...), and date_part('SECOND', ...).
  • Verified the SQL results against Spark while requiring native Comet execution.
  • Ran the SQL test suite with Spark 4.1.

@YutaLin

YutaLin commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Hi @parthchandra, could you help me review this? Thanks!

cc @andygrove

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement TimeType support - Time extraction (from TimeType to Int/Decimal)

2 participants