fix: use session timezone for timestamp subtraction - #25094
Conversation
github-actions
Bot
added
optimizer
There was a problem hiding this comment.
🟡 Changes recommended
The claimed nested-subquery coverage does not currently exercise subtraction inside a subquery.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes #13212 by applying the session timezone when coercing mixed timezone-aware and timezone-naive timestamp subtraction.
Changes:
- Propagates session timezone through type coercion, simplification, and subqueries.
- Preserves timezone metadata while normalizing timestamp precision.
- Adds analyzer, physical-expression, and SQL regression tests.
File summaries
| File | Description |
|---|---|
datafusion/sqllogictest/test_files/datetime/timestamps.slt |
Tests mixed timestamp subtraction across timezones. |
datafusion/optimizer/src/utils.rs |
Uses the new rewriter constructor. |
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs |
Supplies session timezone during expression coercion. |
datafusion/optimizer/src/scalar_subquery_to_join.rs |
Uses the new rewriter constructor. |
datafusion/optimizer/src/analyzer/type_coercion.rs |
Implements timezone-aware subtraction coercion and propagation. |
datafusion/core/tests/expr_api/mod.rs |
Tests direct physical-expression creation. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25094 +/- ##
========================================
Coverage 81.91% 81.92%
========================================
Files 1132 1132
Lines 421117 421323 +206
Branches 421117 421323 +206
========================================
+ Hits 344961 345151 +190
- Misses 55767 55782 +15
- Partials 20389 20390 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| 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) |
There was a problem hiding this 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':
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)|
|
||
| fn coerce(expr: Expr, schema: &DFSchema) -> Result<Expr> { | ||
| let mut expr_rewrite = TypeCoercionRewriter { schema }; | ||
| let mut expr_rewrite = TypeCoercionRewriter::new(schema); |
There was a problem hiding this comment.
This caller does not get the session timezone, so the new rule does not apply here. See comment above.
| if op != &Operator::Minus { | ||
| return None; | ||
| } | ||
| let session_time_zone = self.session_time_zone?; |
There was a problem hiding this comment.
Can we add a test to assert the current/expected behavior when datafusion.execution.time_zone is None? Something like:
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.)
| ), | ||
| _ => return None, | ||
| }; | ||
| let DataType::Timestamp(unit, _) = comparison_coercion(left_type, right_type)? |
There was a problem hiding this comment.
Could we match on the two units directly, or make timeunit_coercion visible and call it instead of going through comparison_coercion?
| ---- | ||
| 0 days 8 hours 0 mins 0.000000000 secs | ||
|
|
||
| # The session timezone, not the aware operand's timezone, controls the cast. |
There was a problem hiding this comment.
The rule seems to diverge for - and =. On this branch:
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.
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
Thanks for pushing 0390171 — moving the rule into BinaryTypeCoercer and making = and - agree both look right to me, and the coverage in timestamps.slt is good.
A few small things inline.
Separately: the folds inlined out of get_coerce_type_for_list / get_coerce_type_for_case_when leave both of those pub functions with zero callers in the workspace, still encoding the pre-PR rule. They're exported from datafusion_expr::type_coercion::other, so an external caller now gets a coercion the planner no longer uses, and IN has two places to keep in sync. I've put a two-commit branch up against your fork that routes them through = instead, with tests and an upgrade-guide note: kumarUjjawal#3.
The PR description now contradicts the code in three places, and it's the description rather than the code that I think is wrong:
- "Comparison operators retain their existing behavior and should be handled separately" —
0390171coerces comparisons as well as subtraction. That's the right call, but the description says the opposite. - "Preserves the timezone of the timezone-aware operand" —
timestamp_types_with_session_timezonereturnsTimestamp(unit, Some(session_time_zone))for both operands, andtest_timestamp_session_timezone_coercionpins that (America/New_Yorkvs naive becomes+08:00). It's semantically a no-op, since relabelling an aware timestamp preserves the instant, but the comment you added attimestamps.slt:2010says the accurate thing and the description doesn't. - "There are no public API changes" —
BinaryTypeCoercer::with_session_time_zoneis new public API.
One more that GitHub won't let me anchor inline, because datafusion/common/src/config.rs isn't in the diff: datafusion.execution.time_zone's doc is still
/// The default time zone
///
/// Some functions, e.g. `now` return timestamps in this time zone
This PR gives the setting a second meaning, so it'd be worth extending that doc and regenerating configs.md with ./dev/update_config_docs.sh. Careful: the description is asserted verbatim at information_schema.slt L448, L559 and L566 — miss those three and sqllogictest CI goes red. An entry in docs/source/library-user-guide/upgrading/56.0.0.md would be worth it too: the default is None, so nothing changes for anyone who leaves it unset, but results move for anyone who sets it.
| @@ -244,7 +244,7 @@ fn evaluate_expr_with_null_column<'a>( | |||
| } | |||
|
|
|||
| fn coerce(expr: Expr, schema: &DFSchema) -> Result<Expr> { | |||
There was a problem hiding this comment.
This answers the question I raised last round about why no session timezone is threaded in here — worth writing down rather than leaving the omission to be rediscovered:
| fn coerce(expr: Expr, schema: &DFSchema) -> Result<Expr> { | |
| /// No session timezone is threaded in here on purpose. This runs over a plan the | |
| /// `TypeCoercion` analyzer has already coerced, so every binary operand pair | |
| /// already shares a type; the only type this helper introduces is the `Null` of | |
| /// the dummy column in `evaluate_expr_with_null_column`, and `Null` against a | |
| /// timestamp is short-circuited by `null_coercion` before the aware/naive rule. | |
| fn coerce(expr: Expr, schema: &DFSchema) -> Result<Expr> { |
| let mut expr_rewrite = TypeCoercionRewriter { | ||
| schema: new_plan.schema(), | ||
| }; | ||
| let mut expr_rewrite = TypeCoercionRewriter::new(new_plan.schema()); |
There was a problem hiding this comment.
Same here — the reasoning is sound, it's just invisible:
| let mut expr_rewrite = TypeCoercionRewriter::new(new_plan.schema()); | |
| // No session timezone: this builds a *searched* `CASE WHEN`, so the | |
| // `CASE expr WHEN` comparison rule never applies, and the only type | |
| // introduced is the untyped `Null` of the HAVING arm. The expressions | |
| // themselves come from a plan `TypeCoercion` has already coerced. | |
| let mut expr_rewrite = TypeCoercionRewriter::new(new_plan.schema()); |
| ) | ||
| .ok_or_else(|| { | ||
| internal_datafusion_err!( | ||
| "Failed to coerce types {expr_type} and {high_type} in BETWEEN expression" |
There was a problem hiding this comment.
This arm coerces low_coerced_type against high_type, but the message reports expr_type — the type it didn't coerce here. With the two-step fold, expr_type and low_type have already been reconciled, so naming all three is what actually tells you which pair failed:
| "Failed to coerce types {expr_type} and {high_type} in BETWEEN expression" | |
| "Failed to coerce types {expr_type}, {low_type} and {high_type} in BETWEEN expression" |
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
test_timestamp_session_timezone_coercion only exercises Some(tz); the None path — the one that has to stay exactly as it was in 55.0.0 — is covered only end to end in the slt. A unit test alongside this one pins the part that's easy to regress:
#[test]
fn test_timestamp_without_session_timezone_coercion() -> Result<()> {
let aware_ms = DataType::Timestamp(Millisecond, Some("America/New_York".into()));
let aware_ns = DataType::Timestamp(Nanosecond, Some("America/New_York".into()));
let naive = DataType::Timestamp(Nanosecond, None);
// Comparisons fall back to reading the naive side in the aware side's
// timezone, in either operand order.
for op in [Operator::Eq, Operator::Gt] {
for (lhs, rhs) in [(&aware_ms, &naive), (&naive, &aware_ms)] {
let (lhs, rhs) = BinaryTypeCoercer::new(lhs, &op, rhs)
.with_session_time_zone(None)
.get_input_types()?;
assert_eq!((lhs, rhs), (aware_ns.clone(), aware_ns.clone()));
}
}
// `Minus` only agrees with that when the units differ and force a coercion.
// At a shared unit the operands are left alone and arrow subtracts their
// raw values — which is exactly the behaviour this PR exists to correct.
let (lhs, rhs) = BinaryTypeCoercer::new(&aware_ms, &Operator::Minus, &naive)
.with_session_time_zone(None)
.get_input_types()?;
assert_eq!((lhs, rhs), (aware_ns.clone(), aware_ns.clone()));
let (lhs, rhs) = BinaryTypeCoercer::new(&aware_ns, &Operator::Minus, &naive)
.with_session_time_zone(None)
.get_input_types()?;
assert_eq!((lhs, rhs), (aware_ns.clone(), naive.clone()));
// A session timezone doesn't disturb pairs that are both aware or both
// naive: those already denote the same kind of value.
let (lhs, rhs) = BinaryTypeCoercer::new(&aware_ms, &Operator::Minus, &aware_ms)
.with_session_time_zone(Some("+08:00"))
.get_input_types()?;
assert_eq!((lhs, rhs), (aware_ms.clone(), aware_ms));
let (lhs, rhs) = BinaryTypeCoercer::new(&naive, &Operator::Minus, &naive)
.with_session_time_zone(Some("+08:00"))
.get_input_types()?;
assert_eq!((lhs, rhs), (naive.clone(), naive));
Ok(())
}That second Minus assertion is the interesting one: Timestamp(ns, Some(tz)) - Timestamp(ns, None) still coerces nothing without a session timezone, which is the 55.0.0 behaviour left intact.
github-actions
Bot
added
documentation
Reuse session-aware comparison coercion across comparison forms and recurse through encoded and nested timestamp types. Document the public API changes and cover the behavior with unit and SQL logic tests. Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
d49d3d1 to
8208a55
Compare
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Which issue does this PR close?
datafusion.execution.time_zoneis not used for basic time zone inference #13212.Rationale for this change
When a timezone-aware timestamp is subtracted from a timezone-naive timestamp, DataFusion does not use
datafusion.execution.time_zoneto interpret the naive value.For example, with the session timezone set to
+08:00, subtracting2024-11-01 00:00:00from2024-11-01 00:00:00+00:00returns zero instead of eight hours.PostgreSQL and DuckDB handle this by implicitly casting the timezone-naive operand to the session timezone. DataFusion already produces the expected result when that cast is written explicitly, so the missing behavior belongs in type coercion.
What changes are included in this PR?
This PR:
timestamptz - timestampexpressions using the session timezone.ExprSimplifier::coerce, ensuringSessionContext::create_physical_exprbehaves consistently with SQL planning.The change is limited to timestamp subtraction. Comparison operators retain their existing behavior and should be handled separately.
The existing DST-boundary limitation described in #25084 remains. Because this PR inserts the previously missing cast automatically, mixed timestamp subtraction can now encounter that limitation for ambiguous or nonexistent local times.
When no session timezone is configured, existing behavior is unchanged.
What is the testing strategy for this PR?
The added tests cover:
datafusion.execution.time_zoneis not used for basic time zone inference #13212.SessionContext::create_physical_exprAPI.The following checks pass:
The extended workspace test suite from the contributor guide also passes, including all 512 SQL logic test files.
Are there any user-facing changes?
There are no public API changes.