adriangb opened a new pull request, #25240: URL: https://github.com/apache/datafusion/pull/25240
## Which issue does this PR close? - Part of https://github.com/apache/datafusion/issues/13212. It does not close it: the session time zone is not consulted yet, this only makes `-` agree with `=` and with itself. ## Rationale for this change Subtracting a timezone-naive timestamp from a timezone-aware one gives a different answer depending on the *time unit* of the aware operand, and disagrees with `=` on the same pair of values. No session time zone is involved. On `main`: ```sql CREATE TABLE t AS SELECT arrow_cast('2024-11-01T00:00:00-04:00', 'Timestamp(Nanosecond, Some("America/New_York"))') AS ts_tz_ns, arrow_cast('2024-11-01T00:00:00-04:00', 'Timestamp(Millisecond, Some("America/New_York"))') AS ts_tz_ms, '2024-11-01T00:00:00'::timestamp AS ts; SELECT ts_tz_ns - ts, ts_tz_ms - ts, ts_tz_ns = ts FROM t; -- main: 4 hours | 0 hours | true ``` The two values compare equal yet are four hours apart, and the answer flips with the storage unit. PostgreSQL 17 and DuckDB 1.5.2, with the session time zone set to `America/New_York` (the only zone in play here, since DataFusion has no session zone set), both return `00:00:00` and `true`. The cause is in `BinaryTypeCoercer::signature_inner`. For `Minus`, the first thing the arithmetic arm does is ask arrow for a result type. `Timestamp(u, Some(tz)) - Timestamp(u, None)` at *equal* units is the one mixed pair arrow can subtract directly, so no cast is inserted and arrow subtracts the raw values, reading the naive operand as UTC. Every other unit pairing fails that probe and falls through to `temporal_coercion_strict_timezone`, which casts the naive operand to the aware operand's zone, which is also what `=` and the other comparisons do. ## What changes are included in this PR? For `Minus` on a mixed timezone-aware / timezone-naive pair, coerce through `temporal_coercion_strict_timezone` before the arrow probe, so every unit pairing inserts the same cast. The naive operand is then read in the aware operand's time zone at every unit, as `=` already reads it. Nothing else changes: pairs that are both aware or both naive, and pairs of two different aware zones, are untouched. ## What is the testing strategy for this PR? - `test_timestamp_minus_mixed_timezone_awareness` in `datafusion/expr-common/src/type_coercion/binary/tests/arithmetic.rs` pins the coerced types for both operand orders at equal and differing units, and the untouched cases. - An slt block in `datafusion/sqllogictest/test_files/datetime/timestamps.slt` with the reproduction above, both operand orders, and the plan showing the inserted cast. Each expectation is the value PostgreSQL and DuckDB return. - The full sqllogictest suite passes; no other expectation changes. ## Are there any user-facing changes? Yes, documented in the 56.0.0 upgrade guide: `Timestamp(u, Some(tz)) - Timestamp(u, None)` at equal units now reads the naive operand in `tz` rather than as UTC. With the values above, `ts_tz - ts` changes from 4 hours to 0 hours, matching `ts_tz = ts` being true. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
