andygrove opened a new issue, #5061:
URL: https://github.com/apache/datafusion-comet/issues/5061
### 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 <interval>` | `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 <interval>` 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
