Skip to content

Commit 8ea0dbf

Browse files
authored
fix: date_part sub-second parts and result nullability, reusing arrow kernels (#26686)
## Which issue does this PR close? - Closes #26678. - Part of #14738. Its reproducer is a `Duration`, which now returns NULL instead of Postgres's 0, so the semantics question stays open there. - Closes #26685. ## Rationale for this change These fixes depend on each other. #26678 and #14738 come from the same [NULL-as-zero branch](https://github.com/apache/datafusion/blob/dad9063becc0b10550936425d8b433046c5622e3/datafusion/functions/src/datetime/date_part.rs#L490-L499). Fixing the #14738 overflow makes `millisecond` and `microsecond` of a long duration return NULL instead of a wrapped value. Without the #26685 fix, that NULL fails with `declared as non-nullable` on a non-nullable input, so fixing the overflow alone would replace a wrong result with an error. ## What changes are included in this PR? - `millisecond`, `microsecond` and `nanosecond` share one function that reuses arrow's `date_part` and `cast` where they compute the right value. - Timestamps and times compute these parts in one pass, instead of two `date_part` calls and a `binary`. - A value that overflows the return type is NULL, as arrow already does for durations. - The integer parts of `date_part` and the Spark `date_part` are always nullable. `epoch` keeps the input's nullability. - The [dev list](https://lists.apache.org/thread.html/90cqgs9dotp39tcwr5ophvb2ftvjc43p) agreed with NULL on overflow and with the nullable results. - c920a5b and 8d5b5f1 add benchmark cases before the changes they measure, so the benchmark bot can compare against them. <img width="1600" height="1380" alt="date-part-subsecond-bot" src="https://github.com/user-attachments/assets/2f5dab7d-6eb5-4a7f-b16e-92a625ff8584" /> The benchmark bot measured `Duration` and `Interval` at 3.4x to 17.2x faster, times at 2.0x to 5.1x, timestamps at 1.9x to 2.8x, and `Timestamp(Second)` at 1.2x to 1.6x, in both the default and the aligned runs. <details><summary>Numbers</summary> Speedups over c920a5b (the bench commit) on 1000 rows, with the bot's thin LTO. The aligned runs add `-align-all-functions=6 -align-all-nofallthru-blocks=6`. The chart and this table show `nanosecond`. The linked runs also have `millisecond` and `microsecond`. | Case | [Neoverse-V2](#26686 (comment)) | [aligned](#26686 (comment)) | [Xeon 8581C](#26686 (comment)) | [aligned](#26686 (comment)) | |---|---|---|---|---| | `nanosecond`, `Duration(ns)`, large values | 12.11x | 12.20x | 13.12x | 12.38x | | `nanosecond`, `Duration(ns)`, under 2.1 s | 3.86x | 3.94x | 4.87x | 4.03x | | `nanosecond`, `Interval(DayTime)` | 9.60x | 9.58x | 6.31x | 6.40x | | `nanosecond`, `Interval(MonthDayNano)` | 6.47x | 6.48x | 8.34x | 8.26x | | `nanosecond`, `Time64(ns)` | 2.36x | 2.23x | 2.48x | 2.69x | | `nanosecond`, `Timestamp(ns)` | 2.01x | 1.98x | 2.33x | 2.26x | | `nanosecond`, `Timestamp(ns)` with a timezone | 2.40x | 2.46x | 2.39x | 2.75x | | `second`, `Timestamp(ns)` (unchanged) | 1.00x | 1.00x | 0.98x | 1.00x | The unchanged `second` cases range from 0.96x to 1.03x across the four runs, so differences in that range are noise. </details> ## What is the testing strategy for this PR? - New cases in `datafusion/sqllogictest/test_files/datetime/date_part.slt`. - `DESCRIBE` queries for the result nullability in `datafusion/sqllogictest/test_files/datetime/date_part.slt` and `datafusion/sqllogictest/test_files/spark/datetime/date_part.slt`. ## Are there any user-facing changes? `EXTRACT(MILLISECOND/MICROSECOND/NANOSECOND)` on `Duration` and `Interval` return correct results. A `Duration` whose total in the requested unit overflows the return type returns NULL, as the other `Duration` parts already do, instead of a wrapped value or a panic. The integer results of `date_part` and the Spark `date_part` are 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.
1 parent 910e8ee commit 8ea0dbf

5 files changed

Lines changed: 669 additions & 139 deletions

File tree

‎datafusion/functions/benches/datetime_expressions/date_part.rs‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,36 @@ fn generate_duration_nanosecond_array_with_nulls(
188188
)
189189
}
190190

191+
fn generate_timestamp_ns_array_with_nulls(rng: &mut StdRng) -> TimestampNanosecondArray {
192+
TimestampNanosecondArray::from(
193+
(0..BATCH_SIZE)
194+
.map(|_| {
195+
if rng.random_bool(NULL_DENSITY) {
196+
None
197+
} else {
198+
Some(rng.random_range(0..TS_BOUND * 1_000_000_000))
199+
}
200+
})
201+
.collect::<Vec<_>>(),
202+
)
203+
}
204+
205+
fn generate_time64_nanosecond_array_with_nulls(
206+
rng: &mut StdRng,
207+
) -> Time64NanosecondArray {
208+
Time64NanosecondArray::from(
209+
(0..BATCH_SIZE)
210+
.map(|_| {
211+
if rng.random_bool(NULL_DENSITY) {
212+
None
213+
} else {
214+
Some(rng.random_range(0..SEC_DAY * 1_000_000_000))
215+
}
216+
})
217+
.collect::<Vec<_>>(),
218+
)
219+
}
220+
191221
fn bench_date_part(
192222
c: &mut Criterion,
193223
udf: &Arc<ScalarUDF>,
@@ -244,6 +274,14 @@ fn criterion_benchmark(c: &mut Criterion) {
244274
let duration_ns_nulls =
245275
Arc::new(generate_duration_nanosecond_array_with_nulls(&mut rng)) as ArrayRef;
246276

277+
let ts_ns_tz =
278+
Arc::new(generate_timestamp_ns_array(&mut rng).with_timezone("America/New_York"))
279+
as ArrayRef;
280+
let ts_ns_nulls =
281+
Arc::new(generate_timestamp_ns_array_with_nulls(&mut rng)) as ArrayRef;
282+
let time64_ns_nulls =
283+
Arc::new(generate_time64_nanosecond_array_with_nulls(&mut rng)) as ArrayRef;
284+
247285
let udf = date_part();
248286

249287
for part in ["year", "month", "week", "day", "hour", "minute"] {
@@ -365,6 +403,41 @@ fn criterion_benchmark(c: &mut Criterion) {
365403
);
366404
}
367405

406+
// Durations past `i32::MAX` milliseconds or microseconds overflow `Int32`, so
407+
// only `nanosecond` runs on them
408+
for (part, return_type) in [
409+
("millisecond", DataType::Int32),
410+
("microsecond", DataType::Int32),
411+
("nanosecond", DataType::Int64),
412+
] {
413+
let mut arrays = vec![
414+
("ns_tz", &ts_ns_tz),
415+
("ns_nulls", &ts_ns_nulls),
416+
("time32_ms", &time32_ms),
417+
("time64_ns", &time64_ns),
418+
("time64_ns_nulls", &time64_ns_nulls),
419+
("interval_dt", &interval_dt),
420+
("interval_mdn", &interval_mdn),
421+
("duration_ns_small", &duration_ns_small),
422+
];
423+
if part == "nanosecond" {
424+
arrays.extend([
425+
("duration_ns", &duration_ns),
426+
("duration_ns_nulls", &duration_ns_nulls),
427+
]);
428+
}
429+
for (name, array) in arrays {
430+
bench_date_part(
431+
c,
432+
&udf,
433+
&format!("date_part_{part}_{name}_1000"),
434+
part,
435+
Arc::clone(array),
436+
return_type.clone(),
437+
);
438+
}
439+
}
440+
368441
for part in ["quarter", "isoyear", "doy", "dow", "isodow"] {
369442
bench_date_part(
370443
c,

0 commit comments

Comments
 (0)