Repository navigation
fix: date_part sub-second parts and result nullability, reusing arrow kernels - #26686
Open
mbutrovich wants to merge 5 commits into
Open
mbutrovich wants to merge 5 commits into
mbutrovich wants to merge 5 commits into
Conversation
…r times, intervals, durations and timezones
…ECOND) for Duration and Interval
github-actions
Bot
added
sqllogictest
Contributor
Author
|
run benchmark datetime_expressions env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
baseline:
ref: c920a5b92df0172b23026c3773dc7cd0fbdb8ce6 |
Contributor
Author
|
run benchmark datetime_expressions env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
baseline:
ref: c920a5b92df0172b23026c3773dc7cd0fbdb8ce6
resources:
arch: amd64 |
Contributor
Author
|
run benchmark datetime_expressions env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: c920a5b92df0172b23026c3773dc7cd0fbdb8ce6 |
Contributor
Author
|
run benchmark datetime_expressions env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: c920a5b92df0172b23026c3773dc7cd0fbdb8ce6
resources:
arch: amd64 |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-extract-nanosecond-duration-interval (adab085) to c920a5b diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
baseline:
ref: "c920a5b92df0172b23026c3773dc7cd0fbdb8ce6"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-extract-nanosecond-duration-interval (adab085) to c920a5b diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: "c920a5b92df0172b23026c3773dc7cd0fbdb8ce6"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-extract-nanosecond-duration-interval (adab085) to c920a5b diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
baseline:
ref: "c920a5b92df0172b23026c3773dc7cd0fbdb8ce6"
resources:
arch: "amd64"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-extract-nanosecond-duration-interval (adab085) to c920a5b diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: "c920a5b92df0172b23026c3773dc7cd0fbdb8ce6"
resources:
arch: "amd64"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix-extract-nanosecond-duration-interval (adab085) to c920a5b diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
baseline:
ref: "c920a5b92df0172b23026c3773dc7cd0fbdb8ce6"
resources:
arch: "amd64"CPU Details (lscpu)Details
Resource Usagedatetime_expressions — base (merge-base)
datetime_expressions — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix-extract-nanosecond-duration-interval (adab085) to c920a5b diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
baseline:
ref: "c920a5b92df0172b23026c3773dc7cd0fbdb8ce6"CPU Details (lscpu)Details
Resource Usagedatetime_expressions — base (merge-base)
datetime_expressions — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix-extract-nanosecond-duration-interval (adab085) to c920a5b diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: "c920a5b92df0172b23026c3773dc7cd0fbdb8ce6"CPU Details (lscpu)Details
Resource Usagedatetime_expressions — base (merge-base)
datetime_expressions — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix-extract-nanosecond-duration-interval (adab085) to c920a5b diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_(second|millisecond|microsecond|nanosecond)_"
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: "c920a5b92df0172b23026c3773dc7cd0fbdb8ce6"
resources:
arch: "amd64"CPU Details (lscpu)Details
Resource Usagedatetime_expressions — base (merge-base)
datetime_expressions — branch
File an issue against this benchmark runner |
mbutrovich
marked this pull request as ready for review
October 23, 2026 22:27
mbutrovich
marked this pull request as draft
October 23, 2026 22:33
mbutrovich
marked this pull request as ready for review
October 23, 2026 22:50
comphead
reviewed
Oct 24, 2026
comphead
left a comment
comphead
left a comment
Contributor
There was a problem hiding this comment.
Thanks for tying the three issues together. Reusing arrow's kernels for intervals and durations is a clear improvement, and the bench commit makes the speedups easy to check. A few points inline. Small ones:
- The doc on
subsecond_partstill explains Int64 by the 60-second bound. After this PRnanosecondon aDurationis its total length, up toi64::MAX, so that sentence covers only half of the cases now. - Coverage gaps worth one query each:
nanosecondon aMonthDayNanopast minus one minute (only -1.5 s is tested, and the% 60_000_000_000arm is new),nanosecondon a negativeDayTimeand one with a non-zero days field,Timestamp(Microsecond)sub-second parts, andTime32/Time64sub-second parts on an array with a NULL row.seconds_with_fractionis now the only path for times, and every existing time test is a single non-null literal.
Comment on lines
+1512
to
+1515
| # Reproducer from https://github.com/apache/datafusion/issues/14738: 2.4e9 microseconds don't fit in Int32 | ||
| query I | ||
| SELECT date_part('microsecond', timestamp '1970-01-01T00:40:00' - timestamp '1970-01-01T00:00:00') | ||
| ---- |
Contributor
There was a problem hiding this comment.
This is the query from #14738, which asks for the Postgres result. In Postgres timestamp - timestamp is an interval and microsecond is the seconds field within the minute, so the issue expects 0. Here the difference is a Duration, and microsecond is its total length. Any duration longer than about 35.8 minutes is then NULL, and millisecond is NULL past about 24.8 days.
That matches how arrow treats the other Duration parts, so I think it is defensible. But it is a third answer to the issue, after the wrapped value and the panic. Should this be Part of #14738, with the semantics question left open? Or is NULL the agreed outcome? If total length is the intended meaning, a follow-up could return Int64 for Duration input, since return_field_from_args sees the input type. That would avoid most of these NULLs.
Comment on lines
+534
to
+541
| (Duration(Nanosecond), DatePart::Nanosecond) => { | ||
| Arc::new(as_duration_nanosecond_array(array)?.reinterpret_cast::<Int64Type>()) | ||
| } | ||
| // arrow's cast returns NULL on overflow, like its `date_part` | ||
| (Duration(_), DatePart::Nanosecond) => Arc::new( | ||
| as_duration_nanosecond_array(&cast(array, &Duration(Nanosecond))?)? | ||
| .reinterpret_cast::<Int64Type>(), | ||
| ), |
Contributor
There was a problem hiding this comment.
These two arms can be one:
(Duration(_), DatePart::Nanosecond) => {
cast(&cast(array, &Duration(Nanosecond))?, &DataType::Int64)?
}arrow's cast returns the input as is when the types already match. It reinterprets Duration(Nanosecond) as Int64 without copying, and it already turns overflow into NULL for the unit change. That removes an arm and the repeated as_duration_nanosecond_array(..)?.reinterpret_cast::<Int64Type>() pair.
Comment on lines
+544
to
+548
| ( | ||
| Interval(ArrowIntervalUnit::DayTime | ArrowIntervalUnit::MonthDayNano) | ||
| | Duration(_), | ||
| _, | ||
| ) => date_part(array, part)?, |
Contributor
There was a problem hiding this comment.
Could this be Interval(_) | Duration(_)? Today Interval(YearMonth) with millisecond or microsecond falls through to seconds_with_fraction, whose doc says it is for timestamps and times. It then fails on its first date_part(.., Second), so extract(millisecond ..) reports Second does not support: Interval(YearMonth) (the slt at L1288 and L1291 pins this). arrow's YearMonth kernel rejects every sub-second part with the requested name, so the message would be accurate. One of those two query error cases would then be enough.
Comment on lines
573
to
+576
| let secs = date_part(array, DatePart::Second)?; | ||
| // This assumes array is primitive and not a dictionary | ||
| let secs = as_int32_array(secs.as_ref())?; | ||
| let subsecs = date_part(array, DatePart::Nanosecond)?; | ||
| let subsecs = as_int32_array(subsecs.as_ref())?; | ||
|
|
||
| // Special case where there are no nulls. | ||
| if subsecs.null_count() == 0 { | ||
| let r: Int64Array = binary(secs, subsecs, |secs, subsecs| { | ||
| (secs as i64) * 1_000_000_000 + (subsecs as i64) | ||
| })?; | ||
| Ok(Arc::new(r)) | ||
| } else { | ||
| // Nulls in secs are preserved, nulls in subsecs are treated as zero to account for the case | ||
| // where the number of nanoseconds overflows. | ||
| let r: Int64Array = secs | ||
| .iter() | ||
| .zip(subsecs) | ||
| .map(|(secs, subsecs)| { | ||
| secs.map(|secs| { | ||
| let subsecs = subsecs.unwrap_or(0); | ||
| (secs as i64) * 1_000_000_000 + (subsecs as i64) | ||
| }) | ||
| }) | ||
| .collect(); | ||
| Ok(Arc::new(r)) | ||
| } | ||
| let fraction = date_part(array, part)?; |
Contributor
There was a problem hiding this comment.
For timestamps and times this makes three passes. Two date_part calls each run a chrono conversion per row, plus a timezone lookup per row and a timezone string parse per call when a timezone is set. Then binary allocates a third values buffer and a third bitmap for the null union. This matches the bench, where timestamps stay at about 1.1x while intervals and durations gain 3.6x to 41x.
timezone_part_typed above already has the one-pass shape. One unary_opt over as_datetime_with_timezone::<T> (or as_datetime::<T> without a timezone), computing dt.second() * PER_SECOND + dt.nanosecond() / (1_000_000_000 / PER_SECOND), would do one conversion per row and one allocation. main already used two passes, so a follow-up is fine. I have not measured it. The ns_tz case you added is where I'd expect it to show most.
| /// `epoch` is NULL only for a NULL input, but the integer parts can be NULL | ||
| /// for a non-null input, so they are always nullable. | ||
| #[test] | ||
| fn return_field_nullability() { |
Contributor
There was a problem hiding this comment.
The integer-part cases here are already exercised end to end by the non-null reproducers at the end of the slt. The ones from #26685 fail on main with declared as non-nullable. The only new information is that epoch follows the input. Could this move to the slt with the existing DESCRIBE idiom (see describe.slt)? For example DESCRIBE SELECT extract(epoch from ts) AS e, extract(second from ts) AS s FROM (VALUES (timestamp '2020-01-01')) t(ts), once with and once without a NULL row. That shows the declared nullability directly.
Comment on lines
+84
to
88
| fn return_field_from_args(&self, _args: ReturnFieldArgs) -> Result<FieldRef> { | ||
| // Nullable like the integer parts of `date_part`, which this simplifies | ||
| // to, because they can be NULL for a non-null input | ||
| Ok(Arc::new(Field::new(self.name(), DataType::Int32, true))) | ||
| } |
Contributor
There was a problem hiding this comment.
With the input no longer read, this override now does exactly what the trait default does. ScalarUDFImpl::return_field_from_args builds Field::new(self.name(), self.return_type(..)?, true). So return_type could return Ok(DataType::Int32) and the override could go.
Another option is to delegate to datafusion_functions::datetime::date_part().return_field_from_args(args), which keeps the two matching by construction. That would also fix a mismatch this PR does not touch. nanosecond and epoch pass through simplify unchanged, but Spark declares Int32 while date_part returns Int64 and Float64.
Either way, the new test checks Int32 and nullable directly and then again against date_part. The comparison alone is enough. Since DESCRIBE shows the planned schema before simplification, it could also be a DESCRIBE SELECT date_part('SECOND', ts) ... query in spark/datetime/date_part.slt instead of a unit test.
Comment on lines
+1265
to
+1271
| # Reproducer from https://github.com/apache/datafusion/issues/26678 | ||
| query III | ||
| SELECT extract(nanosecond from arrow_cast(1500000000, 'Duration(Nanosecond)')), | ||
| extract(nanosecond from interval '1.5 seconds'), | ||
| extract(microsecond from interval '1.5 seconds') | ||
| ---- | ||
| 1500000000 1500000000 1500000 |
Contributor
There was a problem hiding this comment.
These repeat queries added above. Column 1 is L1424, and columns 2 and 3 are L1201-L1202. In the next reproducer, the first column is L1245. Could the issue links go on those queries instead?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
EXTRACT(NANOSECOND)onDurationandIntervalcounts whole seconds twice #26678.date_partreturning wrong results due to overflows #14738.date_partdeclares a non-nullable result for non-null input, but can return NULL #26685.Rationale for this change
These fixes depend on each other. #26678 and #14738 come from the same NULL-as-zero branch. Fixing #14738 makes
millisecondandmicrosecondof a long duration return NULL instead of a wrapped value. Without the #26685 fix, that NULL fails withdeclared as non-nullableon a non-nullable input, so fixing #14738 alone would replace a wrong result with an error.What changes are included in this PR?
millisecond,microsecondandnanosecondshare one function that reuses arrow'sdate_partandcastwhere they compute the right value.date_partand the Sparkdate_partare always nullable.epochkeeps the input's nullability.The benchmark bot measured
DurationandIntervalat 3.6x to 41.6x faster, times at 1.5x to 2.2x, and timestamps within noise.Numbers
Speedups over c920a5b (the bench commit) on 1000 rows, with the bot's thin LTO. "Nulls" means 10% of rows are NULL. The aligned runs add
-align-all-functions=6 -align-all-nofallthru-blocks=6.nanosecond,Duration(ns), large valuesnanosecond,Duration(ns), large values, nullsnanosecond,Duration(ns), under 2.1 snanosecond,Interval(DayTime)nanosecond,Interval(MonthDayNano)millisecond,Interval(DayTime)microsecond,Duration(ns), under 2.1 smicrosecond,Time64(ns)millisecond,Timestamp(ns)nanosecond,Timestamp(ns)(unchanged)second,Timestamp(ns)(unchanged)In the default Xeon run the unchanged
secondcases are 0.98x to 1.08x, so gains in that range on timestamps are noise.What is the testing strategy for this PR?
datafusion/sqllogictest/test_files/datetime/date_part.slt.datafusion/functions/src/datetime/date_part.rs.datafusion/spark/src/function/datetime/date_part.rs.Are there any user-facing changes?
EXTRACT(MILLISECOND/MICROSECOND/NANOSECOND)onDurationandIntervalreturn correct results. ADurationwhose total in the requested unit overflows the return type returns NULL, as the otherDurationparts already do, instead of a wrapped value or a panic. The integer results ofdate_partand the Sparkdate_partare nullable even for non-null input. There are no public API changes.AI Disclosure
I wrote this PR with help from Claude but understand and support the core approach, implementation, and test coverage.