Repository navigation
Commit 8ea0dbf
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
- src/datetime
- spark/src/function/datetime
- sqllogictest/test_files
- datetime
- spark/datetime
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
188 | 188 | | |
189 | 189 | | |
190 | 190 | | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
191 | 221 | | |
192 | 222 | | |
193 | 223 | | |
| |||
244 | 274 | | |
245 | 275 | | |
246 | 276 | | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
247 | 285 | | |
248 | 286 | | |
249 | 287 | | |
| |||
365 | 403 | | |
366 | 404 | | |
367 | 405 | | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
368 | 441 | | |
369 | 442 | | |
370 | 443 | | |
| |||
0 commit comments