andygrove commented on PR #5135: URL: https://github.com/apache/datafusion-comet/pull/5135#issuecomment-5441458737
> **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. Good find, and the fix is in the right place. Matching Spark's omission of the `days` field is the kind of detail that would be very easy to get wrong, and I like that the test explicitly covers days-only intervals, since that is where the omission is observable as a collision. Two things I would like to see addressed. **This will be wrong on Spark 4.3** The comment says SPARK-58236 starts including `days` in the hash from Spark 4.3. So as written, the native hasher is correct for every Spark version Comet supports today and silently wrong for the next one, and the failure mode is a hash mismatch, which means wrong shuffle partitioning and wrong hash join results rather than an error. Comet's convention for this is to resolve the version-dependent policy in Scala (`ShimCometConf`) and pass the resolved boolean through the protobuf, rather than branching per version in Rust. Could this follow that pattern, with an `include_days` flag on the hash expression? Even if 4.3 is not supported yet, wiring the flag now is much cheaper than discovering the mismatch later, and there is already a `spark-4.2` bring-up in flight. At minimum, if you would rather defer, please file an issue and reference it in the comment so this does not get found the hard way. **Nanosecond-to-microsecond conversion** `value.nanoseconds / 1_000` is an integer division that truncates toward zero. That is exact as long as the nanoseconds field is always a multiple of 1000, which it should be since Spark's `CalendarInterval` is microsecond-based. But nothing enforces that, and for a negative value with a non-multiple the truncation direction differs from a floor, giving a hash that does not match Spark. More importantly, is there a range concern? Spark stores `CalendarInterval.microseconds` as an `i64`. Arrow's `IntervalMonthDayNano` stores nanoseconds, also `i64`, so any interval longer than roughly 292 years cannot round-trip. If that truncation happens earlier in the pipeline, the hash here is computed on an already-wrong value. Is that a pre-existing limitation that is documented somewhere? If so, pointing at it from this macro would help. If not, it is worth knowing whether `make_interval` can produce one. -- 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]
