parthchandra commented on PR #6354:
URL:
https://github.com/apache/datafusion-comet/pull/6354#issuecomment-5919287174
A few review notes, comparing against Spark's `DateTimeUtils.truncTimestamp`:
- **[correctness] `native/spark-expr/src/kernels/temporal.rs:143`** — the
`LocalResult::None => resolve_local_datetime(...)` branch is still wrong for
date-level truncation (WEEK/MONTH/QUARTER/YEAR) when a spring-forward gap
straddles midnight. It shifts local time forward by the whole gap length, which
is right for DAY/HOUR/MINUTE but not the date levels. Spark resolves those with
`atStartOfDay`, which returns the gap's end instant, not midnight minus the
pre-gap offset. They agree only when the gap begins exactly at midnight.
Example: `America/Toronto`, `date_trunc('WEEK', ts)` on `1919-03-31
00:45:00-04:00` (gap ran 23:30 Mar 30 to 00:30 Mar 31). Spark returns
`00:30:00-04:00`; this returns `01:00:00-04:00`, which is 30 minutes past the
gap and actually later than the input instant. For the date levels, please
resolve a gap to the transition boundary (the first valid instant at or after
the truncated midnight) instead of the gap-length shift. (The bot flagged the
same case.)
- **[tests] `native/spark-expr/src/kernels/temporal.rs:1235`** — every DST
case here (LA, Sao Paulo) has a gap that starts at midnight, so the date-level
gap rule is never tested where it differs. Please add a straddles-midnight case
(the Toronto 1919 one is clean) for WEEK/MONTH/QUARTER/YEAR, and an
ambiguous-midnight zone like `America/Havana` (fall-back on 2020-11-01) to pin
the earlier-offset rule for date-level overlaps.
- **[tests] `native/spark-expr/src/kernels/temporal.rs:1236`** —
`test_timestamp_trunc_matches_spark_at_dst_transitions` landed between the
Denver doc comment and the `#[test]` it documents, so
`test_timestamp_trunc_dst_boundary` lost its doc and the Denver comment now
describes the wrong test. Move the new test above that comment block.
--
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]
