andygrove commented on PR #4898: URL: https://github.com/apache/datafusion-comet/pull/4898#issuecomment-5004672704
Nice, clean approach to interval support. I traced the micros-to-nanos conversion through all four paths (both writers, both readers) and they are consistent, and the child-vector `getInterval` chain and slicing both look correct. Limiting the scope to pass-through with no native arithmetic feels right. A few questions, none blocking: - Does an interval column survive a native Comet shuffle? The tests cover localTableScan and codegen round-trips but not an exchange, and this PR makes the type eligible for shuffle. A shuffle test, or an explicit note that shuffle is out of scope for this PR, would help. - The codegen adds struct-field and array-element `getInterval` cases, but the tests are all scalar. Could you add a nested (struct or array) interval test to cover those branches? - The codegen output uses `Math.multiplyExact(micros, 1000L)`, which throws on overflow, while the `ArrowWriter` path relies on Spark's writer, which multiplies without the exact check. For an interval whose microseconds exceed what Arrow's int64 nanoseconds can hold, one path throws and the other wraps. Worth a short comment on the intended behavior for that (rare) case. Also heads up: the red `rust-test` check is a runner disk-space flake (`No space left on device` during linking), not a real failure, so it just needs a re-run. _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]
