peterxcli commented on PR #5292: URL: https://github.com/apache/datafusion-comet/pull/5292#issuecomment-5442092755
@andygrove Thanks for the second pass — replies inline, plus one doc improvement pushed. **Upstream vs. fork:** The tagged-struct representation is Comet-specific, so this can't be an upstream fix. In `datafusion-spark`, `SparkMakeInterval::return_type` is hard-coded to `Interval(MonthDayNano)` — the 1000x range loss *is* the return type, not an implementation detail behind it. Fixing it means returning a struct whose "this is a CalendarInterval" meaning lives in field metadata that only Comet's Arrow conversion and FFI layers know to map back to Spark's `CalendarIntervalType`; DataFusion has no such logical type, and other upstream consumers expect the Arrow-native interval. The kernel also needs Comet's per-expression ANSI `fail_on_error` and exact `Decimal(18,6)` seconds. The `Float64`-seconds precision loss *is* separately fixable upstream, and I'm happy to file a `datafusion-spark` issue for that half. I've added this rationale to the PR description. **`toArrowType` callers:** Audited repo-wide. The only production caller is the fallthrough inside `toArrowField` itself (`Utils.scala:235`), which can't reach the `CalendarIntervalType` arm because `toArrowField` matches that type first (line 214). The only other reference is the `UtilsSuite` test that pins the exception. So no caller can hit the throw today; it exists purely as a guard for future direct callers, per your earlier review's suggestion, and its message names `toArrowField` as the fix. **Cross-version wire format:** Not a supported scenario, because it can't arise: the Comet jar is fixed per Spark application, so all executors in one app run the same Comet build, and shuffle blocks (external shuffle service included) are application-scoped — nothing written by one app's Comet is ever read by a different version. Comet persists no Arrow-format state beyond shuffle lifetime; durable storage is Parquet. I've added a note to the description rather than the migration guide. **Dispatch path:** It's intentionally gone for `make_interval` — `CometMakeInterval` moved from `CometCodegenDispatch` to a fully-native `CometExpressionSerde`, so fallback is now straight to Spark with no codegen-dispatch route. The fixture deletion was the "removed" option from your first review. The pure-Spark path is still exercised by every remaining query, since the SQL-file harness computes each expected answer with Comet disabled. I've stated the dispatch-path removal explicitly in the description. **Doc comment:** Good call on `isCalendarIntervalStructField` — added a Scaladoc stating that shape alone is insufficient and the `SPARK::calendarInterval::struct` marker is what prevents a user's own `struct<months:int, days:int, microseconds:bigint>` from being misread. -- 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]
