Repository navigation
Conversation
andygrove
changed the title
date_trunc
andygrove
marked this pull request as ready for review
July 14, 2026 09:12
Contributor
|
run benchmarks date_trunc |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing auto-opt/date_trunc-datafusion-20260713-184055 (bce729e) to 3a29d6b (merge-base) diff using: date_trunc File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagedate_trunc — base (merge-base)
date_trunc — branch
File an issue against this benchmark runner |
Contributor
|
🤔 As main does not have the benchmarks, the bot cannot perform the comparison. I run the benches myself and can replicate locally, so it's all good 👍 |
gabotechs
approved these changes
Jul 14, 2026
Comment on lines
+638
to
658
| fn civil_from_days(days: i64) -> (i64, i64, i64) { | ||
| let z = days + DAYS_EPOCH_SHIFT; | ||
| let era = z.div_euclid(DAYS_PER_ERA); | ||
| let day_of_era = z.rem_euclid(DAYS_PER_ERA); | ||
| let year_of_era = (day_of_era - day_of_era / 1460 + day_of_era / 36524 | ||
| - day_of_era / 146_096) | ||
| / 365; | ||
| let day_of_year = | ||
| day_of_era - (365 * year_of_era + year_of_era / 4 - year_of_era / 100); | ||
| // Month index with March as 0, so that the leap day falls at the end of the year. | ||
| let month_index = (5 * day_of_year + 2) / 153; | ||
| let day = day_of_year - (153 * month_index + 2) / 5 + 1; | ||
| let month = if month_index < 10 { | ||
| month_index + 3 | ||
| } else { | ||
| _date_trunc_coarse::<NaiveDateTime>(granularity, None)?; | ||
| Ok(None) | ||
| } | ||
| month_index - 9 | ||
| }; | ||
| let year = year_of_era + era * 400 + i64::from(month <= 2); | ||
| (year, month, day) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
This math looks correct. Is there any link we can put in the comment so that people and robots can have a reference in case this code needs some maintenance?
For example, I found these:
Contributor
|
Nice, thanks @andygrove! |
Member
Author
|
Thanks for the review @gabotechs! |
kosiew
pushed a commit
to kosiew/datafusion
that referenced
this pull request
Aug 12, 2026
## Which issue does this PR close?
N/A
## Rationale for this change
Optimize existing expression.
## What changes are included in this PR?
Replaced the per-row chrono NaiveDateTime round trip in the untimezoned
coarse path of date_trunc (week/month/quarter/year) with integer
civil-calendar arithmetic, cutting several allocationless-but-validating
chrono field setters down to a few integer ops per row.
## Are these changes tested?
Existing tests.
Benchmark (criterion):
- date_trunc_month_nanos_1000: 48.452% faster (base 19164ns -> cand
9878ns)
- date_trunc_week_nanos_1000: 76.045% faster (base 23337ns -> cand
5590ns)
- date_trunc_month_second_1000: 47.688% faster (base 17376ns -> cand
9090ns)
- date_trunc_quarter_nanos_1000: 32.493% faster (base 22005ns -> cand
14854ns)
- date_trunc_year_nanos_1000: 39.449% faster (base 20460ns -> cand
12388ns)
- date_trunc_minute_1000: 1.2% faster (base 654ns -> cand 647ns)
Full criterion output:
```text
date_trunc_minute_1000 time: [645.50 ns 652.66 ns 659.52 ns]
change: [−2.0837% −1.1996% −0.2678%] (p = 0.01 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mild
date_trunc_month_second_1000
time: [9.0984 µs 9.1139 µs 9.1298 µs]
change: [−47.773% −47.688% −47.597%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severe
date_trunc_week_nanos_1000
time: [5.5882 µs 5.6103 µs 5.6308 µs]
change: [−76.154% −76.045% −75.951%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) low mild
date_trunc_month_nanos_1000
time: [9.8640 µs 9.8831 µs 9.9059 µs]
change: [−48.576% −48.452% −48.309%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe
date_trunc_quarter_nanos_1000
time: [14.849 µs 14.857 µs 14.869 µs]
change: [−32.666% −32.493% −32.339%] (p = 0.00 < 0.05)
Performance has improved.
Found 16 outliers among 100 measurements (16.00%)
1 (1.00%) low severe
2 (2.00%) low mild
7 (7.00%) high mild
6 (6.00%) high severe
date_trunc_year_nanos_1000
time: [12.385 µs 12.391 µs 12.398 µs]
change: [−39.498% −39.449% −39.405%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
2 (2.00%) low mild
4 (4.00%) high mild
3 (3.00%) high severe
```
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
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?
N/A
Rationale for this change
Optimize existing expression.
What changes are included in this PR?
Replaced the per-row chrono NaiveDateTime round trip in the untimezoned coarse path of date_trunc (week/month/quarter/year) with integer civil-calendar arithmetic, cutting several allocationless-but-validating chrono field setters down to a few integer ops per row.
Are these changes tested?
Existing tests.
Benchmark (criterion):
Full criterion output:
Are there any user-facing changes?