Repository navigation
fix: calculate total seconds from interval fields for extract(epoch) - #19807
Merged
Merged
Conversation
Jefffrey
approved these changes
Jan 14, 2026
Jefffrey
left a comment
Contributor
There was a problem hiding this comment.
Results seem consistent with DuckDB 👍
|
|
||
| fn epoch(array: &dyn Array) -> Result<ArrayRef> { | ||
| const SECONDS_IN_A_DAY: f64 = 86400_f64; | ||
| const DAYS_PER_MONTH: f64 = 30_f64; |
Contributor
There was a problem hiding this comment.
I suppose there's no easier way to define this for intervals 🤔
Contributor
Author
There was a problem hiding this comment.
Yes! I just followed the PostgreSQL-way for simplicity here, what do u think?
adriangb
approved these changes
Jan 15, 2026
martin-g
approved these changes
Jan 15, 2026
lemorage
force-pushed
the
hotfix/interval
branch
from
January 16, 2026 03:00
7d2c798 to
d73f19a
Compare
Contributor
|
Good to go 👍 |
Contributor
|
Thanks both! |
de-bgunter
pushed a commit
to de-bgunter/datafusion
that referenced
this pull request
Mar 24, 2026
apache#19807) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#19799. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> The epoch function incorrectly used `date_part(array, DatePart::Second)` for intervals which only extracts the seconds component of the interval structure (0 for "15 minutes"), not the total seconds from all components. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> - Modified epoch() function in date_part.rs to properly handle interval types by extracting struct fields and calculating total seconds - Added regression tests in date_part.slt ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> Yes. Added tests verifying correct conversion for all interval types and precision levels. ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> Yes. `extract(epoch from interval)` now returns correct total seconds instead of 0. <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
This was referenced Oct 23, 2026
mbutrovich
added a commit
to mbutrovich/datafusion
that referenced
this pull request
Oct 23, 2026
…6676) ## Which issue does this PR close? - Closes apache#26673. ## Rationale for this change The old code was complicated because it reused a helper written for timestamps. apache#12514 sent `Duration` epochs through [`seconds`](https://github.com/apache/datafusion/blob/c76ed5396ad3f4a4dc9d9ceeddc419a146b871f9/datafusion/functions/src/datetime/date_part.rs#L507-L547) and added `% 1_000_000_000` and a NULL-as-zero branch to recover the fraction, which works only while the total fits in `i32`. apache#19807 fixed the same bug for `Interval` and left `Duration` on that path. ## What changes are included in this PR? The `Duration` arms of `epoch` now divide the `i64` value by the unit's factor with `unary`, like the `Timestamp` arms, and `seconds` is gone. The first commit adds two cases to the `datetime_expressions` bench without the fix, so the benchmark bot can compare against it. It is also 2.5x to 12x faster on the benchmark bot. `main` made three passes over the batch and fell back to a row-by-row iterator on any NULL or large value. The new loop takes the same time whatever the values or NULLs. <img width="1600" height="868" alt="epoch-duration-bot" src="https://github.com/user-attachments/assets/c70a3f6e-81d6-40ba-ab2d-0cbd832643fc" /> <details><summary>Numbers and codegen</summary> Speedups over `353e4cee5` (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`. | Case | [Neoverse-V2](apache#26676 (comment)) | [aligned](apache#26676 (comment)) | [Xeon 8581C](apache#26676 (comment)) | [aligned](apache#26676 (comment)) | |---|---|---|---|---| | `duration_ns` | 5.60x | 5.57x | 8.09x | 6.95x | | `duration_ns_nulls` | 7.11x | 7.31x | 12.08x | 10.68x | | `duration_ns_small` | 2.50x | 2.45x | 4.50x | 3.53x | | `ns` (unchanged) | 1.00x | 1.01x | 1.09x | 1.03x | In the default Xeon run the unchanged cases are 1.09x to 1.19x, so that run overstates the gain by about 15%. Timing each arrow kernel of `main`'s path separately on 8192 rows on an Apple M5 Max accounts for its totals. | Values | `date_part(Second)` | `date_part(Nanosecond)` | Combine | Sum | |---|---|---|---|---| | Under 2.1 s, `binary` | 2.94 us | 2.32 us | 2.44 us | 7.70 us | | Large, iterator `collect` | 2.92 us | 12.86 us | 16.47 us | 32.25 us | LLVM merges the new loops with the existing `Timestamp` and `Time64` ones, so this PR adds no new loop code. On aarch64 the loop handles 8 rows per iteration with `scvtf.2d` and `fdiv.2d`. On x86_64 the division is packed (`divpd`, `vdivpd` with AVX2) and the `i64` to `f64` conversion is scalar `cvtsi2sd`, because the packed form needs AVX-512DQ. I read this with `cargo asm` (rustc 1.99.0, `release-nonlto`). </details> The code divides rather than multiplying by `1e-9`. The division is most of the loop's time on Neoverse-V2, where `date_part_epoch_s_1000` (no division) takes 412 ns against 1.21 us for `ns`. The multiply would be faster, but it changes 14% of `Duration(ms)` and 41% of `Duration(ns)` results, for example 12.92335 s becomes `12.923350000000001`. ## What is the testing strategy for this PR? New cases in `datafusion/sqllogictest/test_files/datetime/date_part.slt` under `test_extract_duration` cover the `i32::MAX` ns boundary, each `Duration` unit, the issue's reproducer, negative values, NULLs mixed with values, and `i64` extremes. ## Are there any user-facing changes? `EXTRACT(EPOCH)` on `Duration` returns correct results. 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.
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?
select extract(epoch from ('15 minutes')::interval)gives wrong result #19799.Rationale for this change
The epoch function incorrectly used
date_part(array, DatePart::Second)for intervals which only extracts the seconds component of the interval structure (0 for "15 minutes"), not the total seconds from all components.What changes are included in this PR?
Are these changes tested?
Yes. Added tests verifying correct conversion for all interval types and precision levels.
Are there any user-facing changes?
Yes.
extract(epoch from interval)now returns correct total seconds instead of 0.