peterxcli commented on PR #4715: URL: https://github.com/apache/datafusion-comet/pull/4715#issuecomment-5447408531
@andygrove Thanks for the careful pass — pushed a2349c5 addressing it. Point by point: **Removing `CalendarIntervalType` from `supportedDataType` / the `supportedSinkDataType` OR** — good catch, and these two were really one mistake. `CalendarIntervalType` was deliberately added to `supportedDataType` by #4898 with test coverage, and removing it narrowed the hash-expression, scalar-subquery, and nested-type gates — all out of scope for this PR. The `|| isTypeSupported(...)` in `CometSink` existed only to patch calendar intervals back into sinks after that removal (main already has an explicit YM/DT arm there). I've restored `supportedDataType` and reverted `CometSink` to main exactly, so this PR no longer touches either predicate and the conflict with #5025 goes away. `CometLiteral` now only adds `YearMonthIntervalType` alongside the existing `DayTimeIntervalType` arm. **Duplicate `planner.rs` arms with #5161** — confirmed the overlap; the two arms are semantically identical in both PRs. This PR needs them for its literal-input test (`INTERVAL '1-2' YEAR TO MONTH * i`), so I'd propose whichever merges first wins and the other rebases — it's a mechanical one-hunk resolution. Happy to rebase this one if #5161 lands first. **Expression map ordering** — that section of the map is grouped by function family rather than alphabetically (e.g. `Hour` after `LastDay`, `TimestampAdd` in the middle of the M's), so instead of alphabetizing I moved `MultiplyYMInterval` to sit with the interval cluster, directly after `MultiplyDTInterval`, and left `Minute` as is. **Version coverage** — ran `CometSqlFileTestSuite` locally on `-Pspark-3.4`, `-Pspark-3.5`, and the default Spark 4.1 profile; all green, including this fixture and #4898's `calendar_interval.sql`. The overflow case is version-stable by construction: the dispatched code throws the JDK's own `Math.multiplyExact` "integer overflow" `ArithmeticException` on every Spark version, which is what `expect_error(overflow)` matches. -- 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]
