adriangb commented on code in PR #25094: URL: https://github.com/apache/datafusion/pull/25094#discussion_r3968558417
########## datafusion/sqllogictest/test_files/datetime/timestamps.slt: ########## @@ -1980,6 +1980,60 @@ SELECT '2000-01-01T00:00:00'::timestamp - '2010-01-01T00:00:00'::timestamp; ---- -3653 days 0 hours 0 mins 0.000000000 secs +# Regression test for https://github.com/apache/datafusion/issues/13212 +statement ok +SET TIME ZONE = '+08' + +query ??? +WITH timestamps(ts_tz, ts) AS ( + VALUES ('2024-11-01T00:00:00+00:00'::timestamptz, '2024-11-01T00:00:00'::timestamp) +) +SELECT + '2024-11-01T00:00:00+00:00'::timestamptz - '2024-11-01T00:00:00'::timestamp, + ts_tz - ts, + ts - ts_tz +FROM timestamps; +---- +0 days 8 hours 0 mins 0.000000000 secs 0 days 8 hours 0 mins 0.000000000 secs 0 days -8 hours 0 mins 0.000000000 secs + +# The session timezone is propagated when coercing a scalar subquery. +query ? +SELECT ( + SELECT ts_tz - ts + FROM ( + VALUES ('2024-11-01T00:00:00+00:00'::timestamptz, '2024-11-01T00:00:00'::timestamp) + ) AS timestamps(ts_tz, ts) +); +---- +0 days 8 hours 0 mins 0.000000000 secs + +# The session timezone, not the aware operand's timezone, controls the cast. Review Comment: The rule seems to diverge for `-` and `=`. On this branch: ```sql SET datafusion.execution.time_zone = '+08:00'; CREATE TABLE t AS SELECT arrow_cast('2024-11-01T04:00:00Z', 'Timestamp(Nanosecond, Some("America/New_York"))') AS ts_tz, '2024-11-01T00:00:00'::timestamp AS ts; SELECT ts_tz = ts, ts_tz - ts FROM t; ``` returns `true` and `0 days 12 hours`: `=` reads `ts` in `America/New_York` (so the two values are the same instant) while `-` reads it in the session timezone `+08:00` (so they are 12 hours apart). Two values that compare equal yet differ by twelve hours. ########## datafusion/optimizer/src/utils.rs: ########## @@ -244,7 +244,7 @@ fn evaluate_expr_with_null_column<'a>( } fn coerce(expr: Expr, schema: &DFSchema) -> Result<Expr> { - let mut expr_rewrite = TypeCoercionRewriter { schema }; + let mut expr_rewrite = TypeCoercionRewriter::new(schema); Review Comment: This caller does not get the session timezone, so the new rule does not apply here. See comment above. ########## datafusion/optimizer/src/analyzer/type_coercion.rs: ########## @@ -434,6 +459,47 @@ impl<'a> TypeCoercionRewriter<'a> { Ok((left_expr, right_expr)) } + /// Coerces the timezone-naive side of timestamp subtraction using the session + /// timezone, matching PostgreSQL and DuckDB. The timezone-aware side keeps its + /// timezone while both operands are widened to the same precision. + fn timestamp_subtraction_input_types( + &self, + left_type: &DataType, + op: &Operator, + right_type: &DataType, + ) -> Option<(DataType, DataType)> { + if op != &Operator::Minus { + return None; + } + let session_time_zone = self.session_time_zone?; + let (left_time_zone, right_time_zone) = match (left_type, right_type) { + ( + DataType::Timestamp(_, Some(left_time_zone)), + DataType::Timestamp(_, None), + ) => ( + Some(Arc::clone(left_time_zone)), + Some(Arc::from(session_time_zone)), + ), + ( + DataType::Timestamp(_, None), + DataType::Timestamp(_, Some(right_time_zone)), + ) => ( + Some(Arc::from(session_time_zone)), + Some(Arc::clone(right_time_zone)), + ), + _ => return None, + }; + let DataType::Timestamp(unit, _) = comparison_coercion(left_type, right_type)? Review Comment: Could we match on the two units directly, or make `timeunit_coercion` visible and call it instead of going through `comparison_coercion`? ########## datafusion/optimizer/src/analyzer/type_coercion.rs: ########## @@ -434,6 +459,47 @@ impl<'a> TypeCoercionRewriter<'a> { Ok((left_expr, right_expr)) } + /// Coerces the timezone-naive side of timestamp subtraction using the session + /// timezone, matching PostgreSQL and DuckDB. The timezone-aware side keeps its + /// timezone while both operands are widened to the same precision. + fn timestamp_subtraction_input_types( + &self, + left_type: &DataType, + op: &Operator, + right_type: &DataType, + ) -> Option<(DataType, DataType)> { + if op != &Operator::Minus { + return None; + } + let session_time_zone = self.session_time_zone?; Review Comment: Can we add a test to assert the current/expected behavior when `datafusion.execution.time_zone` is `None`? Something like: ```sql statement ok RESET datafusion.execution.time_zone statement ok SET datafusion.explain.logical_plan_only = true # With no session timezone the naive operand is still read as UTC: # 2024-11-01T00:00:00-04:00 is 04:00Z, and the naive value is taken as 00:00Z. statement ok CREATE TABLE no_session_tz AS SELECT arrow_cast('2024-11-01T00:00:00-04:00', 'Timestamp(Nanosecond, Some("America/New_York"))') AS ts_tz, '2024-11-01T00:00:00'::timestamp AS ts; query ?? SELECT ts_tz - ts, ts - ts_tz FROM no_session_tz; ---- 0 days 4 hours 0 mins 0.000000000 secs 0 days -4 hours 0 mins 0.000000000 secs query TT EXPLAIN SELECT ts_tz - ts FROM no_session_tz; ---- logical_plan 01)Projection: no_session_tz.ts_tz - no_session_tz.ts 02)--TableScan: no_session_tz projection=[ts_tz, ts] statement ok SET datafusion.explain.logical_plan_only = false ``` (I ran this against the PR branch: the values are 4 hours / -4 hours and no cast is inserted, so it records today's behaviour.) ########## datafusion/optimizer/src/analyzer/type_coercion.rs: ########## @@ -398,9 +418,14 @@ impl<'a> TypeCoercionRewriter<'a> { ) -> Result<(Expr, Expr)> { let left_data_type = left.get_type(left_schema)?; let right_data_type = right.get_type(right_schema)?; - let (left_type, right_type) = + let (left_type, right_type) = if let Some(types) = + self.timestamp_subtraction_input_types(&left_data_type, &op, &right_data_type) Review Comment: Could this go in `BinaryTypeCoercer` instead of the analyzer? This special case lives in `TypeCoercionRewriter`, so the PR has to send the session timezone through four subquery call sites and through `ExprSimplifier::coerce`. One caller still does not get it: the `coerce` function in `optimizer/src/utils.rs`. `BinaryTypeCoercer` in `expr-common` is the one source of coercion rules. The analyzer, the simplifier, the physical `BinaryExpr::data_type`, the statistics solver, and interval arithmetic all use it. A rule in `BinaryTypeCoercer` applies to all of them with no plumbing. The cause of the bug is also in that file. In the arithmetic arm of `signature_inner`, the first branch asks arrow for a result type. Arrow accepts `Timestamp(u, Some) - Timestamp(u, None)` when the units are equal and reads the naive side as UTC. When the units differ, the pair falls through to `temporal_coercion_strict_timezone`, which casts the naive side to the aware side's timezone. This is what makes results depend on the units. A check before the arrow probe fixes that. On `main` with `SET TIME ZONE = '+08:00'`: ```sql SELECT arrow_cast('2024-11-01T00:00:00Z', 'Timestamp(Nanosecond, Some("+08:00"))') - '2024-11-01T00:00:00'::timestamp; -- 0 hours (wrong) SELECT arrow_cast('2024-11-01T00:00:00Z', 'Timestamp(Millisecond, Some("+08:00"))') - '2024-11-01T00:00:00'::timestamp; -- 8 hours (right) ``` -- 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]
