andygrove commented on PR #5292: URL: https://github.com/apache/datafusion-comet/pull/5292#issuecomment-5441615682
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. Fixing the 1000x range loss and the `Decimal(18,6)` to `Float64` precision loss in one go is a real improvement, and representing the interval the way Spark does, as separate months, days, and microseconds, is clearly the right model. Good to see `getSupportLevel` go from `Incompatible` to `Compatible` as a result. The metadata check in `isCalendarIntervalStructField` does require the marker key rather than just the field shape, which I was worried about, so a user's own `struct<months:int, days:int, microseconds:bigint>` will not be misread. Worth saying that explicitly in the doc comment, since it is the property the whole design rests on. Four things. **Should the fix go upstream to `datafusion-spark` instead?** This replaces the `datafusion-spark` `make_interval` wrapper with a Comet-local kernel and drops the dependency. The bugs being fixed, `Float64` seconds and nanosecond storage, exist in the upstream implementation too, so every other `datafusion-spark` consumer still has them. Comet has been moving expressions the other direction, upstreaming native implementations into `datafusion-spark`. Is there a reason this one has to fork rather than fix upstream and pin? If the answer is "the tagged-struct representation is Comet-specific and cannot go upstream", saying so in the description would settle it. **`toArrowType(CalendarIntervalType)` now throws** Turning a working case into an `UnsupportedOperationException` means every caller has to have been converted to `toArrowField`. Did you audit them all, including callers outside `Utils.scala` and in test code? A runtime `UnsupportedOperationException` from a type conversion is not a friendly failure mode, and it will only show up on a path that happens to be exercised. If some callers legitimately cannot produce the metadata, the exception message should say what to call instead in more actionable terms than "requires toArrowField". **Cross-version compatibility of the wire format** The Arrow representation of `CalendarIntervalType` changes from `Interval(MonthDayNano)` to a struct. That affects native shuffle output and anything else that persists or transfers an Arrow schema. Within a single job everything is consistent. What about a rolling upgrade, where some executors run the old Comet and some the new one, or a shuffle service holding blocks written by the previous version? Is that a scenario Comet supports? If it is, this needs a note in the migration guide. If it is not, saying so in the description would close the question. **Scope** 23 files touching serde, FFI, Arrow readers and writers, codegen input and output, Scala UDF codegen, and the native kernel. That is a lot of surface for one review pass, and the two `make_interval_dispatch*.sql` files being deleted means the dispatch path loses its dedicated coverage at the same time as the native path gains it. Is the dispatch path still reachable for `make_interval`? If it is, it still needs a fixture. If it is not, the description should say the dispatch path is gone rather than leaving the deletion unexplained. -- 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]
