apache / apache/datafusion-comet

[EPIC] Complete interval type support: scan, operator gates, and remaining interval expressions

Open
#5,061 0 comments 0 reactions 0 assignees View on GitHub
area:expressions area:scan enhancement
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 6h
Merged PRs (30d)
190

Description

### What is the problem the feature request solves?

#4540 tracked the foundational interval work: map the three Spark interval types to Arrow and get them through the FFI, serde and codegen-dispatch paths. That part has now largely landed via #4898 (CalendarIntervalType) and #4976 (nested values and native shuffle), so #4540's premise ("Comet has no support for Spark's interval data types") no longer describes the state of the tree. This issue supersedes it and tracks what the audit of `1d3044f35` found still missing.

The headline result: the native engine handles `Interval` / `Duration` arrays better than the Scala side admits. Almost every remaining gap is a type gate in Scala, not missing Rust.

#### Verified working today

All of the following ran fully native and matched Spark on `1d3044f35` (Spark 4.1 profile), against Parquet-backed data so the expressions were not constant-folded:

- `make_dt_interval`, `make_ym_interval`, `multiply_dt_interval`
- an interval column through an equi-join, a multi-key sort, `first` / `last`, `coalesce` / `if`, and `=`
- native shuffle carrying an ANSI interval data column

#### Verified broken or falling back

| Area | Behavior on `1d3044f35` |
|---|---|
| `hash` / `xxhash64` of a CalendarInterval | **Query fails**: `Unsupported data type in hasher: Interval(MonthDayNano)` |
| Null CalendarInterval literal | **Query fails** (#5058) |
| Parquet scan of an ANSI interval column | Entire scan falls back |
| Single-column sort on an interval | `Sort` falls back (multi-column sort is not checked and works) |
| `min` / `max` / `sum` / `avg` over intervals | `HashAggregate` falls back |
| `GROUP BY ` | `Exchange` falls back |
| Window with an interval in `PARTITION BY` / `ORDER BY` / the aggregate | `Window` falls back |
| `hash` / `xxhash64` of an ANSI interval | `Project` falls back |
| `-interval`, `abs(interval)` | `Project` falls back, despite native support existing |
| `cast(interval as string)`, `cast(string as interval)` | `Project` falls back |
| `extract` / `date_part` of an interval field | `Project` falls back |
| `date + interval`, `ts + interval`, `ts - ts`, `date - date` | `Project` falls back |
| `interval / n`, `ym_interval * n` | `Project` falls back |

### Describe the potential solution

#### 1. Correctness bugs (do these first)

- [ ] #5059 — `hash` / `xxhash64` of a CalendarInterval aborts the stage. #4898 added `CalendarIntervalType` to `QueryPlanSerde.supportedDataType`, which is the gate `hash.scala` consults, but `native/spark-expr/src/hash_funcs/utils.rs` has no branch for `Interval(MonthDayNano)`.
- [ ] #5058 — a null CalendarInterval literal throws a native exception instead of evaluating to null.

Both are cases where a Scala gate claims support the native side does not have. Worth a sweep of the other `supportedDataType` consumers (`CometScalarSubquery`, `CometSink`) for the same shape of problem.

#### 2. Scan support

- [ ] #5060 — read ANSI interval columns (`YearMonthIntervalType` / `DayTimeIntervalType`) in the native Parquet scan.

This is the gating item for everything else being reachable in real queries. Today the only way to get an interval column into a Comet operator is to construct it inline with `make_dt_interval` / `make_ym_interval`, which is what every existing test does. Note that `CalendarIntervalType` can never appear in a Parquet file (Spark's `ParquetTable.supportsDataType` accepts `AtomicType`; `CalendarIntervalType` extends `DataType` directly while the ANSI types extend `AnsiIntervalType extends AtomicType`), so the current `DataTypeSupport.isTypeSupported` list is inverted: it admits the one interval type that cannot occur and rejects the two that can.

#### 3. Type gates that still exclude intervals

Each of these is a small change plus a test, and each currently costs a fallback:

- [ ] `QueryPlanSerde.supportedScalarSortElementType` already carries a `TODO: Include SparkSQL's YearMonthIntervalType and DayTimeIntervalType`. Single-column sort falls back; multi-column sort is not checked at all and produces correct results, so the restriction looks removable.
- [ ] `AggSerde.minMaxDataTypeSupported` / `avgDataTypeSupported` / `sumDataTypeSupported` reject intervals. Spark supports `min` / `max` / `sum` / `avg` over both ANSI interval types.
- [ ] `CometShuffleExchangeExec.supportedHashPartitioningDataType` and the columnar-shuffle `supportedSerializableDataType` reject all three interval types, so `GROUP BY ` falls back unless shuffle mode is `native`. #4976 added intervals to the native-shuffle predicate only, leaving the two paths asymmetric.
- [ ] Window operator support for interval partition / order / aggregate expressions.
- [ ] `arithmetic.scala`'s `supportedDataType` rejects intervals, so `UnaryMinus` and `Abs` fall back even though `native/spark-expr/src/math_funcs/negative.rs` already has overflow-checked interval negation that is unreachable and untested. Tracked as #4756.
- [ ] `literals.scala` special-cases only `DayTimeIntervalType`; YearMonth and Calendar interval literals are `Unsupported`.

PR #5025 (closes #5021) centralizes these predicates and is the natural place to land several of them coherently rather than one gate at a time.

#### 4. Expressions not yet wired

Now that the types round-trip, most of these are a one-line `CometCodegenDispatch` registration plus a SQL file test.

| Expression | Issue | Open PR |
|---|---|---|
| `make_dt_interval` | #3098 | landed; #4338 predates it |
| `make_ym_interval` | #3100 | landed |
| `multiply_dt_interval` | #3101 (closed) | landed |
| `make_interval` | #3099 | #5030 (dispatch), #5039 (native) |
| `timestampadd` / `timestampdiff` | — | #5030 |
| `multiply_ym_interval` | #3102 | #4715 |
| `divide_dt_interval` | #3096 | #4901 |
| `divide_ym_interval` | needs an issue | — |
| `multiply_interval` / `divide_interval` (legacy CalendarInterval `*` and `/`) | needs an issue | — |
| `extract` / `date_part` of interval fields (`ExtractANSIInterval*`, `ExtractInterval*`) | needs an issue | — |
| `date_add_interval` | #3086 | — |
| `timestamp_add_interval` | #3114 | — |
| `timestamp_add_ym_interval` | #3115 | — |
| `date_add_ym_interval` | needs an issue | — |
| `time_add_interval` | #3121 | — |
| `subtract_timestamps` | #3112 | — |
| `subtract_dates` | #3094 | — |
| `subtract_times` | #3139 | — |
| `datetime_sub` | #3134 | — |
| `try_make_interval` | #3103 | — |
| `cast` interval to/from string | needs an issue | — |

`multiply_dt_interval`, `make_dt_interval` and `make_ym_interval` are done; the rest of the column is the remaining work. Several of the open PRs above were written before the type plumbing landed and may simplify considerably now.

#### 5. Test coverage

Interval coverage today is four SQL files (`calendar_interval.sql`, `make_dt_interval.sql`, `make_ym_interval.sql`, `multiply_dt_interval.sql`), one `CometCodegenSuite` round-trip and one `CometArrowStreamSuite` round-trip. Nothing covers the operator paths that #4898 and #4976 newly unlocked and that I verified do work: join, multi-key sort, `first` / `last`, `coalesce` / `if` / comparison, and native shuffle of ANSI (not just Calendar) intervals. Those are silent-regression risk right now.

- [ ] Operator-level tests for the paths listed above, for all three interval types.
- [ ] Scan round-trip tests once #5060 lands: nulls, negatives, the full `startField` / `endField` range, dictionary encoding, multiple row groups.
- [ ] A gate-consistency test, so a type admitted by `supportedDataType` but unhandled natively is caught at test time rather than as a stage abort. #5059 and #5058 are both instances of that class.

A trap worth documenting for anyone writing these: a single-row `VALUES (make_dt_interval(...))` gets collapsed by `ConvertToLocalRelation` into a folded literal, so the test passes without exercising Comet at all. Derive intervals from Parquet-backed integer columns instead.

#### 6. Docs

- [ ] `expressions.md` marks `extract` and `date_part` as supported with no interval caveat. The `avg`, `try_avg`, `abs`, `*` and `try_add` rows already carry accurate "interval falls back" notes.

### Additional context

- Supersedes #4540, which tracked the foundational type work now delivered by #4898 and #4976.
- Audit performed against `1d3044f35` with the Spark 4.1 profile. Every "verified" claim above was reproduced with temporary SQL-file tests rather than inferred from reading the gates.
- Related: #4418 ([EPIC] date/time expressions), #4506 ([EPIC] codegen dispatch), #5021 / #5025 (centralizing type predicates).

Contributor guide

Open the contributing guide

Research direction

Treat this as an epic and select one unchecked item first. Start with the named Scala gate or entry point, such as QueryPlanSerde, arithmetic.scala, literals.scala, or CometShuffleExchangeExec, then run the relevant SQL-file tests; inspect native/spark-expr paths when the item names them. Done means the selected interval case is supported without fallback or failure and has a regression test, with expressions.md updated for the documentation item.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust, scala, sql
Domain
backend, data-engineering, distributed-systems, documentation, testing
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.