parthchandra commented on PR #4039: URL: https://github.com/apache/datafusion-comet/pull/4039#issuecomment-4314258448
Thank you @mbutrovich. Excellent comments! > > **`div_floor` inconsistency in `unix_timestamp`** > > The NTZ path (and the existing Timestamp path) uses `micros / MICROS_PER_SECOND` for the no-null fast path but `div_floor(micros, MICROS_PER_SECOND)` for the nullable path. For negative timestamps (pre-epoch), these give different results: `-1_500_000 / 1_000_000 = -1` vs `div_floor = -2`. Spark uses `Math.floorDiv` in all cases. This is pre-existing but worth fixing while you're in here. Just use `div_floor` in both branches. > Changed both the NTZ and Timestamp no-null fast paths in unix_timestamp.rs to use div_floor(micros, MICROS_PER_SECOND) instead of micros / MICROS_PER_SECOND, matching Spark's Math.floorDiv for pre-epoch negative timestamps. > **Code duplication in `timestamp_trunc_array_fmt_helper`** > > The NTZ branch in the macro repeats the same format-to-truncation-function mapping that `timestamp_trunc_ntz` already has. Could the macro's NTZ branch delegate to `timestamp_trunc_ntz` directly, or at least share a format lookup? Extracted ntz_trunc_fn_for_format() that resolves a format string to the corresponding NaiveDateTime truncation function. Both timestamp_trunc_ntz and the macro's NTZ branch now use it -- 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]
