apache / apache/datafusion-comet
Math expressions: revisit interval / input-type gating and add regression tests once interval support lands
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
Spun out from items #4 and #7 of #4500 to defer until the in-flight native interval-support PRs land. Both are math expressions whose interval handling becomes relevant only once interval types serialize.
## greatest / least (#4500 item 4)
`Greatest` / `Least` are registered as bare `CometScalarFunction("greatest")` / `CometScalarFunction("least")` with no input-type gating.
Probed on Spark 3.5 and 4.0 across a wide range of input types, comparing Comet output against Spark and recording whether the projection ran natively:
| Input type | Runs natively | Matches Spark |
| --- | --- | --- |
| int / mixed numeric | yes | yes |
| double incl. `NaN` / `-0.0` / `Infinity` | yes | yes |
| decimal (mixed precision/scale) | yes | yes |
| string (default collation) | yes | yes |
| date / timestamp | yes | yes |
| binary | yes | yes |
| array (incl. with-null, nested, array-of-struct) | yes | yes |
| struct (incl. with-null) | yes | yes |
| null-only | yes | yes |
| day-time / year-month interval | no (falls back) | yes |
| collated string (Spark 4.x, e.g. `UTF8_LCASE`) | no (falls back) | yes |
**No live defect today.** Unsupported types (intervals, collated strings) fail serialization and fall back to Spark cleanly: no runtime error, no silent divergence. Note: the original suggestion to gate to scalar sort element types via `supportedScalarSortElementType` would be a **net regression**, since it would disable the array / struct / decimal cases that currently work natively and correctly.
## signum (#4500 item 7)
Spark `Signum` accepts `DoubleType`, `YearMonthIntervalType`, `DayTimeIntervalType` (returning a `Double` sign of the underlying months / micros). Comet registers a bare `CometScalarFunction("signum")` whose native UDF only handles double.
Probed on Spark 3.5:
| Input type | Runs natively | Matches Spark |
| --- | --- | --- |
| double | yes | yes |
| int (cast to double) | yes | yes |
| day-time interval | no (falls back) | yes |
| year-month interval | no (falls back) | yes |
**No live defect today.** Interval inputs fall back gracefully (intervals do not serialize), so there is no runtime error.
## Why this is deferred
There are pending PRs adding native interval support. Once intervals serialize, both `greatest` / `least` and `signum` over interval columns will start reaching their native UDFs instead of falling back, so the correctness / gating story must be re-evaluated at that point. For `signum`, the native path would additionally need to read the interval's integer representation (months for year-month, micros for day-time) and return its sign as a double.
## Deferred work
- [ ] After interval support lands, re-verify `greatest` / `least` over `DayTimeIntervalType` / `YearMonthIntervalType` (correctness vs Spark; add a `getSupportLevel` gate only if a real divergence/error appears).
- [ ] After interval support lands, handle `signum` over interval inputs: either extend the native `signum` path to compute the sign of the underlying integer representation, or add an explicit `getSupportLevel` gate so it falls back cleanly with a reason in EXPLAIN.
- [ ] Add `CometExpressionSuite` regression tests locking in the verified native behavior for both expressions (numerics/NaN, decimal, string, date, timestamp, binary, arrays incl. nested/with-null, structs incl. with-null, null-only for greatest/least; double/int for signum) plus the graceful interval / collated-string fallback.
- [ ] Revisit collated-string handling for `greatest` / `least` under the collation umbrella (#4496) if collated inputs ever become natively supported.
Surfaced by the `audit-comet-expression` skill run in #4486.
Contributor guide
Research direction
Start with CometExpressionSuite and the native greatest, least, and signum entry points after interval serialization lands. Compare interval results with Spark, verify the listed native and fallback cases, and add regression coverage; done means correct interval handling or an explicit getSupportLevel fallback with an EXPLAIN reason.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala
- Domain
- data-engineering, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100