andygrove commented on PR #5030: URL: https://github.com/apache/datafusion-comet/pull/5030#issuecomment-5099104833
Rebased onto main and addressed both rounds. Replies are inline on each thread; summarizing the cross-cutting items here. **`make_interval` is now in this PR** rather than deferred, since @mbutrovich is right that #4898 makes the stated reason stale. It is `object CometMakeInterval extends CometCodegenDispatch[MakeInterval]` plus the `classOf[MakeInterval]` entry, exactly as you both described, and the PR title and description are updated. @peterxcli, on the native route: I looked for `SparkMakeInterval` and found no `make_interval` implementation anywhere under `native/`, so I went with codegen dispatch to match the rest of this PR. If you were thinking of an upstream `datafusion-spark` function, a native path would be a reasonable follow-up, but it would want its own PR given the CalendarInterval ANSI semantics. Per @mbutrovich's note, the ANSI overflow path has its own coverage in `make_interval_ansi.sql`, where `failOnError` is true, alongside `make_interval.sql` for the non-ANSI path where the same input returns NULL instead. **A pre-existing bug surfaced while writing those tests**, filed as #5058. A null `CalendarIntervalType` literal in a projection throws `CometNativeException: Interval(MonthDayNano) is not supported in Comet` instead of falling back or evaluating. #4898 added the type to the support checks, codegen kernels and serialization, but the null-literal match in `planner.rs` has no `Interval` arm, so the plan is accepted and then fails natively. `YearMonthIntervalType` is not in the supported set and falls back cleanly, which is the behavior a null calendar interval should have at minimum. It is easy to hit indirectly: `MakeInterval` is `NullIntolerant`, so `NullPropagation` rewrites `make_interval(NULL, 2, 3, ...)` into a null interval literal and the expression vanishes before Comet sees it. Not caused by this PR, and since literal NULL arguments never exercise the expression anyway, the fixture drives its NULL coverage from nullable columns and points at #5058. @peterxcli, on the non-blocking suggestions: `MILLISECOND` is in both fixtures, `MICROSECOND` is in `timestampdiff`, and both DST boundaries and `TIMESTAMP_NTZ` are covered rather than just one of them. Verified green on all four supported profiles (3.4, 3.5, 4.0, 4.1). One thing worth flagging from that sweep: the alias coverage had to be split by Spark version, because 3.4's grammar rule is only `TIMESTAMPADD | DATEADD` and `TIMESTAMPDIFF | DATEDIFF`, with `DATE_ADD` and `DATE_DIFF` joining in 3.5. Details are in the thread on `expressions.md`. -- 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]
