apache / apache/datafusion-comet

Math expressions: revisit interval / input-type gating and add regression tests once interval support lands

Open
#4,756 0 comments 0 reactions 0 assignees View on GitHub
area:expressions enhancement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.