andygrove opened a new issue, #6193:
URL: https://github.com/apache/datafusion-comet/issues/6193
### What is the problem the feature request solves?
Spark lowers `extract(field FROM interval)` and `date_part('field',
interval)` to one of twelve `ExtractIntervalPart` subclasses. The ANSI types
get `ExtractANSIInterval{Years,Months,Days,Hours,Minutes,Seconds}` and
`CalendarInterval` gets
`ExtractInterval{Years,Months,Days,Hours,Minutes,Seconds}`. None of them has a
serde, so a projection that uses one falls back to Spark.
`ExtractANSIIntervalDays` also sits under ordinary date arithmetic. Spark
rewrites `date + <DAY-precision interval>` to `DateAdd(date,
ExtractANSIIntervalDays(interval))`, and the subtract form to `DateAdd(date,
UnaryMinus(ExtractANSIIntervalDays(interval)))`. That happens in
`Analyzer.ResolveBinaryArithmetic` on 3.4 and 3.5, and in
`BinaryArithmeticWithDatetimeResolver` on 4.0 and later. `DateAdd` is native,
but its argument is not. A literal such as `date + INTERVAL '30' DAY` is
unaffected in practice, because `ConstantFolding` reduces
`ExtractANSIIntervalDays(literal)` to an int. An interval column is affected,
and the commonest one is date subtraction itself: `d1 - d2` returns `INTERVAL
DAY` in the default interval mode, so `d + (d1 - d2)` falls back.
It also limits testing. `CometSqlFileTestSuite` excludes `ConstantFolding`,
so inside a SQL fixture even `date + INTERVAL '1' DAY` falls back, and that
spelling can't be pinned by a fixture until this is fixed.
Reproduced on `main` at `dd68a531c`, default Spark 4.1 profile:
```sql
CREATE TABLE t (d date, d1 date, d2 date, n int) USING parquet;
INSERT INTO t VALUES (date'2024-01-31', date'2024-03-15', date'2024-01-01',
5), (NULL, NULL, NULL, NULL);
SELECT d + (d1 - d2) FROM t; --
extractansiintervaldays is not supported
SELECT d - (d1 - d2) FROM t; --
extractansiintervaldays is not supported
SELECT extract(DAY FROM make_dt_interval(n, 1, 2, 3)) FROM t; --
extractansiintervaldays is not supported
SELECT date_part('HOUR', make_dt_interval(n, 1, 2, 3)) FROM t; --
extractansiintervalhours is not supported
SELECT extract(YEAR FROM make_ym_interval(n, 1)) FROM t; --
extractansiintervalyears is not supported
SELECT extract(DAY FROM make_interval(0, 0, 0, n)) FROM t; --
extractintervaldays is not supported
```
Each comment is the fallback reason Comet reports. The first two queries
also need #5864, which adds the `SubtractDates` serde. On `main` the extractor
is just the first unsupported node the serde reaches.
### Describe the potential solution
All twelve are `UnaryExpression`s whose `doGenCode` is a single
`IntervalUtils.getX(child)` call, returning `IntegerType`, `ByteType` or
`DecimalType(8, 6)`. The codegen dispatcher already reads all three interval
types and writes all three result types, so each one needs only a
`CometCodegenDispatch` registration in `temporalExpressions`, like
`CometMakeDTInterval`. The classes and their result types are the same on
3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0, so no shim is needed.
I checked that locally on #5864's head with the 4.1 profile. With all twelve
registered as `new CometCodegenDispatch[...]`, `expect_dispatch` passes and the
answers match Spark for `d + (d1 - d2)`, `d - (d1 - d2)`, `d +/- INTERVAL '<n>'
DAY` inside the harness, and `extract` of every field of all three interval
types, over Parquet data with negative and NULL values.
The PR for this should:
- Register the twelve extractors.
- Add SQL fixtures with `expect_dispatch(...)` for each field, derived from
Parquet columns. Cover negative values, NULLs, both the `extract` and
`date_part` spellings, and `date +/- (d1 - d2)` and `date +/- INTERVAL '<n>'
DAY` for the `DateAdd` rewrite.
- Update `expressions.md`. The `+` and `-` notes that #5864 adds say a
DAY-precision interval column on a date falls back because
`ExtractANSIIntervalDays` has no serde. The `extract`, `date_part` and
`datepart` rows carry no interval caveat at all today.
### Additional context
Part of #5061, whose table of expressions not yet wired lists "`extract` /
`date_part` of interval fields (`ExtractANSIInterval*`, `ExtractInterval*`)" as
needing an issue. Found while reviewing #5864.
--
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]
