Skip to content

Make the session-timezone comparison rule public, and deprecate the two helpers it orphans - #3

Closed
adriangb wants to merge 5 commits into
kumarUjjawal:fix/13212-timezone-inferencefrom
pydantic:claude/datafusion-pr-25094-merge-weal0o
Closed

adriangb wants to merge 5 commits into
kumarUjjawal:fix/13212-timezone-inferencefrom
pydantic:claude/datafusion-pr-25094-merge-weal0o

Conversation

@adriangb

@adriangb adriangb commented Sep 10, 2026 •

Copy link
Copy Markdown

Which issue does this PR close?

Nothing on its own — this targets your fix/13212-timezone-inference branch, i.e. it is a
follow-up to apache#25094. Merging it updates that PR.

Rationale for this change

0390171 reaches IN and CASE expr WHEN by inlining the folds that used to live in
get_coerce_type_for_list and get_coerce_type_for_case_when. That leaves two things behind:

  1. The rule itself — "IN coerces the way = does, session timezone included" — exists only
    as a private function inside datafusion-optimizer.
  2. Both pub helpers still answer the question "how does IN coerce?", with an answer
    DataFusion itself no longer uses.

Those helpers aren't dead code. A GitHub search turns up two non-fork callers of
get_coerce_type_for_list: Canner/WrenAI, which vendors a copy of this very rewriter,
and waynexia/jsonfusion, which uses it for its own IN-list inference.
get_coerce_type_for_case_when has no external callers I can find.

An earlier version of this PR gave both helpers a session_time_zone parameter instead. That
is a breaking change — api-health.md lists "adding new required parameters" first
among breaking changes, and lists "an internal refactor to make DataFusion more consistent"
among the weak reasons to make one. It also wouldn't have helped those two callers: they'd
pass None and land exactly where they started. Deprecation is non-breaking, gives them a
warning plus a documented replacement, and stays reversible.

What changes are included in this PR?

Two commits:

  1. Give the session-timezone comparison rule one public home. Moves
    comparison_coercion_with_session_timezone out of the analyzer and into expr-common,
    next to comparison_coercion itself, as pub. The analyzer calls it in the same three
    places; it just no longer owns the rule. Adds a unit test covering the None session
    timezone and the Null-versus-dictionary case, where routing through = differs from
    comparison_coercion.

  2. Deprecate get_coerce_type_for_list and get_coerce_type_for_case_when
    (since = "56.0.0"), pointing at that function, with a worked example of the replacement
    fold in the rustdoc and an entry in the 56.0.0 upgrade guide.

The analyzer's coercion behavior is unchanged by both commits — this is API shape only.

Are there any user-facing changes?

No breaking changes. One new public function, two deprecations, and an upgrade-guide entry.
Per the API health policy the deprecated functions stay for six major versions, so nothing
has to move before 62.0.0.

Verified locally: cargo fmt --all -- --check; cargo clippy --all-targets -D warnings and
the unit tests for datafusion-expr-common, datafusion-expr and datafusion-optimizer;
cargo test -p datafusion-expr --doc; and 76 sqllogictest files including
datetime/timestamps.slt.

🤖 Generated with Claude Code

@github-actions github-actions Bot added documentation Improvements or additions to documentation logical-expr optimizer labels Sep 10, 2026
adriangb and others added 2 commits September 10, 2026 12:25
`0390171` added a private `comparison_coercion_with_session_timezone` to the
analyzer so `IN`, `BETWEEN` and `CASE expr WHEN` coerce the way `=` does. That
is the right rule, but it is the only copy of it and it is not reachable from
outside `datafusion-optimizer`.

Move it to `expr-common`, next to `comparison_coercion` itself, and make it
`pub`. Nothing changes for the analyzer, which keeps calling it in the same
three places; it just no longer owns the rule.

Also documents *why* `timestamp_types_with_session_timezone` exists, and covers
the new function with a unit test — including the `None` session timezone and
the `Null`-versus-dictionary case, where routing through `=` differs from
`comparison_coercion`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both are thin folds of `comparison_coercion`, and both are now unreachable from
the planner: `0390171` coerces `IN` and `CASE expr WHEN` through `=` so they
pick up the `datafusion.execution.time_zone` rule, and these two functions have
nowhere to take that setting from.

Leaving them as they are would leave two `pub` functions that still answer the
question "how does `IN` coerce?" with an answer DataFusion itself no longer
uses. Adding a `session_time_zone` parameter would answer it correctly, but
that is a breaking change to a signature with known downstream callers, for a
reason the API health policy lists as weak ("an internal refactor to make
DataFusion more consistent").

So deprecate them in 56.0.0 instead, pointing at
`comparison_coercion_with_session_timezone`. Nothing breaks today, callers get
a compiler warning and a worked example of the replacement fold, and the
decision stays reversible for the six versions before removal.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@adriangb
adriangb force-pushed the claude/datafusion-pr-25094-merge-weal0o branch from 4493734 to 2fbac69 Compare September 10, 2026 17:32
@adriangb adriangb changed the title Coerce IN, BETWEEN and CASE WHEN through the same rule as = Make the session-timezone comparison rule public, and deprecate the two helpers it orphans Sep 10, 2026
@kumarUjjawal
kumarUjjawal force-pushed the fix/13212-timezone-inference branch from d49d3d1 to 8208a55 Compare September 11, 2026 05:50
@adriangb adriangb closed this Oct 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation logical-expr optimizer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants