peterxcli commented on PR #5135:
URL:
https://github.com/apache/datafusion-comet/pull/5135#issuecomment-5442344133
Thanks for the pass — responses to both points.
**Spark 4.3 / `include_days` flag:** I've filed #5498 to track the version
switch. I'd rather not wire the flag in this PR: every supported profile (3.4.3
through 4.2.0 — the SPARK-58236 fix is not on `branch-4.2`) omits `days`, so
the flag would have a single value across all of CI and the `include_days =
true` arm would be untestable dead code until a Spark 4.3 bring-up exists. The
issue captures the `ShimCometConf`-plus-proto design so it's cheap to pick up
as part of 4.3 support, and the code comment next to the SPARK-58236 note
points at it.
**`nanoseconds / 1_000`:** the division is exact by construction. Both
JVM-to-Arrow producers — `ArrowWriters.CalendarIntervalWriter` and the codegen
dispatch kernel — convert with `Math.multiplyExact(microseconds, 1000L)`, so
the nanoseconds field is always an exact multiple of 1000 (the
negative-truncation case can't occur), and an interval whose time component
exceeds ~292 years throws `ArithmeticException` at conversion time instead of
reaching the hasher with a wrapped value. This matches what you verified in
your July review ("recovers the original microseconds exactly or throws rather
than wrapping"). I've added a comment on the macro documenting the invariant
and where it's enforced, per your suggestion.
--
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]
