fix: spark-compatible date_trunc — timestamp return type, NULL on invalid unit, and unit from a column - #2237
fix: spark-compatible date_trunc — timestamp return type, NULL on invalid unit, and unit from a column#2237davidlghellin wants to merge 11 commits into
date_trunc — timestamp return type, NULL on invalid unit, and unit from a column#2237Conversation
Spark's `date_trunc` (TruncTimestamp) always returns TimestampType
regardless of input, but Sail returned `timestamp_ntz` for DATE and
TIMESTAMP_NTZ inputs (it preserved the coerced input's missing timezone).
This made `date_trunc('year', d) = TIMESTAMP '...'` return 0 rows, since
the ntz result never matched a tz-aware literal.
Cast the value argument to `Timestamp(µs, Some(session_tz))` before
truncating, mirroring Spark's coercion to TimestampType. The cast is
gated on date-family types (DATE/TIMESTAMP/TIMESTAMP_NTZ/STRING) so other
types (int, time) still reject like Spark instead of being silently
coerced. For a plain TIMESTAMP column the cast is identity and is elided
by SimplifyExpressions, so there is no runtime overhead in the common path.
Also return a typed-NULL timestamp when the unit is NULL or unrecognized
(Spark returns NULL; DataFusion's date_trunc errors). `is_null_arg` sees
through a `CAST(NULL AS STRING)` wrapper that is not folded at plan time.
Verified against Spark JVM 4.1.1
There was a problem hiding this comment.
Pull request overview
This PR aligns Sail’s date_trunc behavior with Spark (validated against Spark JVM 4.1.1) by enforcing a consistent timestamp return type, returning NULL for NULL/invalid units, and supporting per-row truncation when the unit is supplied from a column (while keeping execution vectorized).
Changes:
- Update the planner to coerce date-family inputs to
timestamp(microsecond, session TZ) and ensuredate_truncalways returns atimestamp. - Add Spark-compatible
NULLbehavior forNULL/unrecognized units and implement a vectorized per-row unit implementation inSparkDateTruncwhen the unit argument is an array. - Expand the Spark compatibility feature tests to cover new type, invalid/NULL unit, and column-unit scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
python/pysail/tests/spark/function/features/date_trunc.feature |
Adds/updates Spark-compat scenarios (return type, invalid/NULL unit behavior, per-row unit from a column). |
crates/sail-plan/src/function/scalar/datetime.rs |
Planner-side Spark semantics: always return timestamp, cast supported inputs to timestamp, and short-circuit invalid/NULL units to typed-NULL. |
crates/sail-function/src/scalar/datetime/spark_date_trunc.rs |
Implements vectorized per-row unit handling by computing truncation once per distinct supported unit and assembling results via interleave. |
Spark 3.5.7 Test ReportCommit Information
Test Summary
Test DetailsError CountsPassed Tests Diff(empty) Failed Tests |
Spark 4.1.1 Test ReportCommit Information
Test Summary
Test DetailsError CountsPassed Tests Diff--- before.txt 2026-07-24 05:52:40.218992420 +0000
+++ after.txt 2026-07-24 05:52:40.515992431 +0000
@@ -2232 +2231,0 @@
-pyspark/sql/tests/connect/test_parity_udtf.py::LegacyArrowUDTFParityTests::test_udtf_with_table_argument_queryFailed Tests(truncated) |
Ibis Test ReportCommit Information
Test Summary
Test DetailsError CountsPassed Tests Diff(empty) Failed Tests |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2237 +/- ##
==========================================
- Coverage 79.45% 78.04% -1.41%
==========================================
Files 909 911 +2
Lines 171731 172405 +674
==========================================
- Hits 136454 134561 -1893
- Misses 35277 37844 +2567
*This pull request uses carry forward flags. Click here to find out more.
... and 77 files with indirect coverage changes 🚀 New features to boost your workflow:
|
expressions and do not accept the same units, but Sail's plan builder feeds both from date_trunc's unit table. Five behaviours diverge: - trunc(date, 'DAY') and trunc(date, 'HOUR') truncate and return the date itself, where Spark returns NULL: a unit finer than a day is not valid for trunc. - an unrecognized unit errors instead of returning NULL. - a NULL unit errors instead of returning NULL. - a unit coming from a column errors, because the plan builder only matches Expr::Literal, so the query falls through to DataFusion's date_trunc, which demands a scalar unit. They live in date_trunc.feature because a single unit table governs both. Ten new scenarios, values captured on Spark JVM 4.1.1: five green, five tagged @sail-bug (strict xfail) and removed by the fix that follows.
date_trunc — timestamp return type, NULL on invalid unit, and unit from a columndate_trunc — timestamp return type, NULL on invalid unit, and unit from a column
shehabgamin
left a comment
There was a problem hiding this comment.
Will re-review after I see the benchmark results!
| AggregateExec: mode=FinalPartitioned, gby=[date_trunc(CASE WHEN Utf8("minute") ILIKE Utf8("mon") OR Utf8("minute") ILIKE Utf8("mm") THEN Utf8("month") WHEN Utf8("minute") ILIKE Utf8("yy") OR Utf8("minute") ILIKE Utf8("yyyy") THEN Utf8("year") WHEN Utf8("minute") ILIKE Utf8("dd") THEN Utf8("day") ELSE Utf8("minute") END,hits.#4)@0 as date_trunc(CASE WHEN Utf8("minute") ILIKE Utf8("mon") OR Utf8("minute") ILIKE Utf8("mm") THEN Utf8("month") WHEN Utf8("minute") ILIKE Utf8("yy") OR Utf8("minute") ILIKE Utf8("yyyy") THEN Utf8("year") WHEN Utf8("minute") ILIKE Utf8("dd") THEN Utf8("day") ELSE Utf8("minute") END,hits.#4)], aggr=[count(Int64(1))] | ||
| RepartitionExec: partitioning=Hash([date_trunc(CASE WHEN Utf8("minute") ILIKE Utf8("mon") OR Utf8("minute") ILIKE Utf8("mm") THEN Utf8("month") WHEN Utf8("minute") ILIKE Utf8("yy") OR Utf8("minute") ILIKE Utf8("yyyy") THEN Utf8("year") WHEN Utf8("minute") ILIKE Utf8("dd") THEN Utf8("day") ELSE Utf8("minute") END,hits.#4)@0], 4), input_partitions=4 | ||
| AggregateExec: mode=Partial, gby=[date_trunc(minute, #4@0) as date_trunc(CASE WHEN Utf8("minute") ILIKE Utf8("mon") OR Utf8("minute") ILIKE Utf8("mm") THEN Utf8("month") WHEN Utf8("minute") ILIKE Utf8("yy") OR Utf8("minute") ILIKE Utf8("yyyy") THEN Utf8("year") WHEN Utf8("minute") ILIKE Utf8("dd") THEN Utf8("day") ELSE Utf8("minute") END,hits.#4)], aggr=[count(Int64(1))] | ||
| ProjectionExec: expr=[CASE WHEN lower(Utf8("minute")) = Utf8("year") OR lower(Utf8("minute")) = Utf8("yyyy") OR lower(Utf8("minute")) = Utf8("yy") THEN date_trunc(Utf8("year"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("quarter") THEN date_trunc(Utf8("quarter"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("month") OR lower(Utf8("minute")) = Utf8("mon") OR lower(Utf8("minute")) = Utf8("mm") THEN date_trunc(Utf8("month"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("week") THEN date_trunc(Utf8("week"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("day") OR lower(Utf8("minute")) = Utf8("dd") THEN date_trunc(Utf8("day"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("hour") THEN date_trunc(Utf8("hour"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("minute") THEN date_trunc(Utf8("minute"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("second") THEN date_trunc(Utf8("second"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("millisecond") THEN date_trunc(Utf8("millisecond"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("microsecond") THEN date_trunc(Utf8("microsecond"),hits.#4) ELSE TimestampMicrosecond(NULL, Some("UTC")) END@0 as #105, count(Int64(1))@1 as #106, CASE WHEN lower(Utf8("minute")) = Utf8("year") OR lower(Utf8("minute")) = Utf8("yyyy") OR lower(Utf8("minute")) = Utf8("yy") THEN date_trunc(Utf8("year"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("quarter") THEN date_trunc(Utf8("quarter"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("month") OR lower(Utf8("minute")) = Utf8("mon") OR lower(Utf8("minute")) = Utf8("mm") THEN date_trunc(Utf8("month"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("week") THEN date_trunc(Utf8("week"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("day") OR lower(Utf8("minute")) = Utf8("dd") THEN date_trunc(Utf8("day"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("hour") THEN date_trunc(Utf8("hour"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("minute") THEN date_trunc(Utf8("minute"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("second") THEN date_trunc(Utf8("second"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("millisecond") THEN date_trunc(Utf8("millisecond"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("microsecond") THEN date_trunc(Utf8("microsecond"),hits.#4) ELSE TimestampMicrosecond(NULL, Some("UTC")) END@0 as CASE WHEN lower(Utf8("minute")) = Utf8("year") OR lower(Utf8("minute")) = Utf8("yyyy") OR lower(Utf8("minute")) = Utf8("yy") THEN date_trunc(Utf8("year"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("quarter") THEN date_trunc(Utf8("quarter"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("month") OR lower(Utf8("minute")) = Utf8("mon") OR lower(Utf8("minute")) = Utf8("mm") THEN date_trunc(Utf8("month"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("week") THEN date_trunc(Utf8("week"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("day") OR lower(Utf8("minute")) = Utf8("dd") THEN date_trunc(Utf8("day"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("hour") THEN date_trunc(Utf8("hour"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("minute") THEN date_trunc(Utf8("minute"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("second") THEN date_trunc(Utf8("second"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("millisecond") THEN date_trunc(Utf8("millisecond"),hits.#4) WHEN lower(Utf8("minute")) = Utf8("microsecond") THEN date_trunc(Utf8("microsecond"),hits.#4) ELSE TimestampMicrosecond(NULL, Some("UTC")) END] |
There was a problem hiding this comment.
Given that this seems to significantly modify the ClickBench query plan, can you please do a benchmark?
There was a problem hiding this comment.
Sure! I'll benchmark the affected ClickBench query (main vs this branch) and post the before/after here.
There was a problem hiding this comment.
Benchmarked the affected ClickBench path (main vs this branch), 1M timestamps, on the projection
whose plan changed.
No regression. The plan diff is cosmetic: ClickBench's unit is the literal 'minute', so the new
11-branch CASE ... date_trunc(...) constant-folds to a single date_trunc('minute', ts) — the
identical physical op as before.
Time (criterion, 1M rows) — within noise, a wash
| projection | time |
|---|---|
old — date_trunc('minute', ts) |
~0.94 ms |
| new — 11-branch CASE (literal unit) | ~0.90 ms |
Allocations (divan): byte-identical — both 8.131 MB allocated / 8.036 MB peak. Same time, same
memory: after folding it's literally the same plan.
Verifiable directly (cargo run --example explain): the literal unit optimizes to 1 date_trunc
call. Self-contained harness (public datafusion pinned =54.0.0, no Sail checkout):
https://github.com/davidlghellin/sail-benchmarks/tree/master/sail-2237-date-trunc
|
@davidlghellin Thanks for doing the benchmark! Here's the initial review from Codex 5.6 ultra |
Thanks @shehabgamin — all five are addressed. Everything below was measured on Spark JVM 4.1.1 and against the PR build, not reasoned about. Before the five: 1. Unit evaluated once — fixed. A simple-CASE selector turned out not to be enough on its own. The dispatch has to tell three outcomes apart — named a granularity / was NULL / matched nothing — and 2. ANSI errors suppressed — fixed, with one correction to the prescription. Spark does not convert the value unconditionally.
There is a second axis: a NULL unit and an unmatched unit differ, and which one skips the value follows the argument order — Sail raises where Spark raises, but still carries DataFusion's parser message instead of 3. Localization — fixed. NTZ and DATE both route through 4. Complex unit types — fixed. Rejected at analysis with 5. Error class — fixed. Other divergences found and fixed while here: Known gaps left in place, each with a scenario: a binary unit that is not valid UTF-8; a scalar subquery Spark's optimizer does not fold; a DATE past year ~294,247 (no reachable input found — literals, Coverage went from 13 scenarios to 153, plus 10 Python tests. All are validated against Spark JVM 4.1.1 except the two plan-snapshot scenarios, which are Sail-only by nature. |
date_truncandtruncdiverged from Spark in several independent ways. All expected values in the new scenarios were validated against Spark JVM 4.1.1.1. It must always return
timestampSpark hardcodes
TruncTimestamp.dataTypetoTimestampTypeand casts an NTZ input ininputTypes([v4.1.1 source(https://github.com/apache/spark/blob/v4.1.1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala)).Sail returned
timestamp_ntzfor an NTZ input, which made a downstream comparison against atimestampsilently drop rows.This related #1717 (issue #1682) rather than contradicting it: that PR fixed
TIMESTAMP → TIMESTAMP; theTIMESTAMP_NTZ → TIMESTAMPleg was still missing.2. An invalid or NULL unit is NULL, not an error
Spark returns NULL when the unit is NULL or unrecognized; DataFusion's
date_truncerrors.3. The unit may come from a column
The systemic one: our Spark-semantics fixes live in the plan builder and match on
Expr::Literal, so when the same value arrives as a column that code never runs and the query falls through to DataFusion'sdate_trunc, which demands a scalar unit. Spark resolves the unit per row.A literal argument is constant-folded, so the pre-existing scenarios never exercised the columnar path at all — they were green with the gap fully intact.
How it works now. A literal string unit is resolved while the plan is built and emits the single call it names, so the common case carries no branch. A unit that only exists as a column is resolved by
SparkTruncLevel, a small UDF that maps it to its granularity, and the plan dispatches on thatwith a
CASE.The unit is resolved in exactly one evaluation, and that is load-bearing rather than an optimization. The dispatch has to tell three outcomes apart — named a granularity / was NULL / matched nothing — and
CASE x WHEN NULLnever matches, so a NULL selector shares theELSEwith an unmatched one. Every way of splitting them in the plan (coalesce,IS NULL) mentions the unit a second time, and DataFusion will not share a volatile subtree between mentions: a unit likeIF(rand() < 0.5, NULL, 'YEAR')would then be drawn once per mention and rows would take a branch neither draw called for.4.
truncis in scope, and it was corrupting datatruncshares the unit table, so it gets the same treatment — and fixing it surfaced the worst bug here:trunc(DATE '2300-01-01', 'YEAR')returned1715-01-01, silently and with no error, on an ordinary date. A DATE reached DataFusion's nanosecond path and the day count wrapped. Truncation now runs entirely in microseconds, asDateTimeUtils.truncTimestampdoes.trunc(1, 'YEAR')returned1970-01-01for the same family of reason — Arrow reinterprets an integer as a day count where Spark rejects the query.Other divergences fixed
resolves the local time with
.single(), which has no answer inside a DST gap or the repeatedhour of a fall-back. DATE and TIMESTAMP_NTZ now localize through
SparkTimestamp/localize_with_fallback. This bites a plain DATE too, in a zone whose clock moves at midnight.Timestamp ... out of range, includingdate_trunc('YEAR', DATE '9999-12-31'), which was tagged as a known bug.toUpperCase(Locale.ROOT);'mınute'(dotless i) and'ſecond'(long s) returned NULL where Spark truncates.2009-2-12and a bare2024; Arrow's demands a fixed-positionYYYY-MM-DD. Sail's ownCAST(… AS TIMESTAMP)already accepted them, so the two surfaces disagreed on the same string.DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPEinstead of being measured as their string form, and the type gate matches on the LOGICAL type, so a dictionary- or run-end-encoded string is not rejected for its encoding.AnalysisException, notIllegalArgumentException.The value conversion follows Spark's codegen, not its interpreter
Whether an unconvertible value raises under ANSI depends on the unit, and the rule is in
TruncInstant.codeGenHelper, which disagrees with the interpretedevalHelper(codegen is thedefault and is what users see):
date_trunc('INVALID', <bad value>)date_trunc(IF(id=0,'INVALID','DAY'), <bad value>)date_trunc(<NULL unit column>, <bad value>)trunc(<bad value>, <NULL unit column>)The last two differ because
nullSafeCodeGenevaluates the left child only:TruncTimestamp(format, ts)skips the timestamp on a NULL format, whileTruncDate(date, format)always evaluates the date. All four are covered.Tests
From 13 scenarios to 153, plus 10 Python tests for what SQL cannot express (dictionary-encoded columns, and the exception class —
Then query erroraccepts any exception whose message matches). All validated against Spark JVM 4.1.1 except the two plan-snapshot scenarios, which are Sail-only by nature.Scenarios pin down what a passing NULL would hide: the session time zone is set explicitly (the harness default is UTC, where a hardcoded UTC would pass), DST overlaps assert
unix_timestampbecause both resolutions render the same wall clock, NULL results assert their type, and error scenarios match Spark's error class rather than.*.The ClickBench plan shrank 8.4 KB: a literal unit now collapses to one call by construction rather than relying on the simplifier.
Known gaps, deliberately left
Each has a scenario, tagged
@sail-bug:CAST_INVALID_INPUT— the ANSI string-cast surface, shared by every function that casts a string.trunchandles it; Sail rejects the value earlier through every construction. That range difference belongs to the CAST/to_datesurface, but it is pinned here because it guards something: were the range widened,truncwould wrap silently past year ~294,247 — the same failure this PR fixes at 2262.The columnar path repeats the value subtree once per branch. Collapsing it would make the value an ordinary argument, always evaluated, which is exactly what the short circuit forbids.