andygrove commented on PR #4715:
URL:
https://github.com/apache/datafusion-comet/pull/4715#issuecomment-5004866382
Thanks for the rebase, this is much cleaner to review now, and the three
points from last round are all addressed well. Moving the YM interval support
into `CometLiteral` rather than the global `supportedDataType` is exactly
right, and the overflow and null-interval test cases are good additions. I
traced the literal path and it is consistent: the literal round-trips as an
`IntervalYearMonth` array through `planner.rs` into the kernel and is read via
`getInt`, and the literal-input query exercises it.
One thing before this is good to go: the `datatypes.md` change flips
`DayTimeIntervalType` to supported, but this PR is YearMonth-only and DayTime
multiply/divide are still open (#4900, #4901), so DayTime is not fully
supported yet. That also contradicts this PR's own `expressions.md` note
("DayTime/Calendar interval forms fall back") and the "Interval types fall back
to Spark today" sentence just above the table. Could you flip only
`YearMonthIntervalType` and leave DayTime at the pending marker, and adjust
that intro line?
Minor, non-blocking: a one-line comment in `CometLiteral.getSupportLevel`
noting that YM interval is deliberately kept out of `supportedDataType` (so it
does not get claimed as flowing through arbitrary native operators) would help
the next reader.
_This review was prepared with the assistance of an LLM (Claude)._
--
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]
