Skip to content

fix: calculate total seconds from interval fields for extract(epoch) - #19807

Merged
adriangb merged 5 commits into
apache:mainfrom
lemorage:hotfix/interval
Jan 16, 2026
Merged

adriangb merged 5 commits into
apache:mainfrom
lemorage:hotfix/interval

Conversation

@lemorage

@lemorage lemorage commented Jan 14, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

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?

  • 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?

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.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Jan 14, 2026

@Jefffrey Jefffrey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose there's no easier way to define this for intervals 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adriangb adriangb left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing! Maybe we could add some more test cases? E.g. extract(epoch from arrow_cast( '100ms'::interval, 'Interval::(MothDayNano)') -> 0.1` and such.

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Jan 16, 2026

@adriangb adriangb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! @Jefffrey any remaining feedback from your end that needs to be addressed?

@Jefffrey

Copy link
Copy Markdown
Contributor

Good to go 👍

@adriangb
adriangb added this pull request to the merge queue Jan 16, 2026
@adriangb

Copy link
Copy Markdown
Contributor

Thanks both!

Merged via the queue into apache:main with commit 5edda9b Jan 16, 2026
30 checks passed
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.
-->
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

select extract(epoch from ('15 minutes')::interval) gives wrong result

4 participants