Repository navigation
Support Preimage for date_trunc - #22114
Conversation
|
I hope to review this soon |
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
|
@alamb 😢 |
please nvm, and don't worry about it 🐱 |
# Conflicts: # datafusion/functions/src/datetime/date_trunc.rs
|
I merged up from main to resolve a conflict |
alamb
left a comment
There was a problem hiding this comment.
Thanks @sdf-jkl -- I am sorry that this got left hanging
I started going through it with claude today
One thing if flagged that I haven't had a chance to review is a wrong results bug with wacky timezones -- maybe we can make the code more conservative
I will try and get this reviewed over the next day or two with a fresh set of 👓
🔴 High — wrong results: hour granularity with a named timezone
has_variable_width_in_tz (date_trunc.rs:145) excludes Day and coarser but permits Hour, so the timestamp arm applies a fixed 3600 s step. That holds for every whole-hour DST transition (I confirmed America/New_York fall-back round-trips exactly — _date_trunc_coarse_with_tz disambiguates by offset), but it breaks when a zone's offset shifts by a non-hour amount. Australia/Lord_Howe shifts 30 minutes:
create table lh as select arrow_cast(column1, 'Timestamp(Nanosecond, Some("Australia/Lord_Howe"))') as ts
from (values ('2024-10-05T15:00:00Z'),('2024-10-05T15:30:00Z'),('2024-10-05T15:45:00Z'),
('2024-10-05T16:00:00Z'),('2024-10-05T16:15:00Z'));
-- ground truth (eq on the cast result, so preimage can't fire): 2 rows
SELECT arrow_cast(ts,'Utf8') FROM lh
WHERE arrow_cast(date_trunc('hour', ts),'Utf8') = '2024-10-06T02:30:00+11:00';
-- 2024-10-06T02:30:00+11:00
-- 2024-10-06T02:45:00+11:00
-- with the preimage rewrite: 4 rows
SELECT arrow_cast(ts,'Utf8') FROM lh
WHERE date_trunc('hour', ts) = arrow_cast('2024-10-05T15:30:00Z','Timestamp(Nanosecond, Some("Australia/Lord_Howe"))');
-- 2024-10-06T02:30:00+11:00
-- 2024-10-06T02:45:00+11:00
-- 2024-10-06T03:00:00+11:00 <-- date_trunc gives 03:00, not 02:30
-- 2024-10-06T03:15:00+11:00 <-- same
The spring-forward bucket is 30 minutes wide, not 60. general_date_trunc(15:30) == 15:30 so the alignment check passes, and upper overshoots by 30 min. Since rewrite_with_preimage replaces the predicate rather than refining it, the interval has to be exact.
| /// variants encode the bare date; Timestamp variants are anchored at the | ||
| /// date's midnight in the given timezone. Returns `None` for unsupported | ||
| /// target types. | ||
| pub(crate) fn date_to_scalar( |
There was a problem hiding this comment.
What do you think about making this a method on ScalarValue (like ScalarValue::try_from_date, matching the existing methods like https://docs.rs/datafusion/latest/datafusion/common/enum.ScalarValue.html#method.try_from_string)?
That might make it easier to discover in the future
There was a problem hiding this comment.
we can do this as a follow on
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22114 +/- ##
==========================================
- Coverage 82.89% 82.88% -0.01%
==========================================
Files 1153 1153
Lines 457487 457758 +271
Branches 457487 457758 +271
==========================================
+ Hits 379213 379404 +191
- Misses 55264 55311 +47
- Partials 23010 23043 +33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
|
@alamb do you think there's value in this? If so I can get back to it. |
|
I do think there is value @sdf-jkl -- what I haven't been able to do is find enough time to reason through it and convince myself it is correct. I had been hoping we would have some more engagement from others in the community as well 😢 |
# Conflicts: # datafusion/functions/src/datetime/date_part.rs # datafusion/functions/src/datetime/date_trunc.rs
|
I got distracted by this PR this morning and spent a while merging it up to main, adding some more tests, and fixing a timezone related issue. Let me know what you think @sdf-jkl -- I am truly sorry it took so long |
| /// | ||
| /// Returns `None` if `target_type` is not a date or timestamp type, or if | ||
| /// midnight on `date` cannot be represented such as when the timezone skips | ||
| /// midnight that day (for example a daylight saving transition at 00:00). |
There was a problem hiding this comment.
How important is this last property (that it returns None for date's with no midnight)? The alternative is to return the first timestamp that does exist in the day, probably 01:00. Admittedly that is rather tricky to calculate just now, but once apache/arrow-rs#11535 is available it'd be much simpler.
With this current design there is always a risk of a sudden NULL appearing in a sorted array of dates so operators that use this function cannot be known to preserve the sorting, possibly requiring resorting of sorted data.
|
Thank you for the review @mhilton -- I think I implemented all your comments. |
| Ok(Arc::new(result)) | ||
| } | ||
|
|
||
| fn date_to_scalar(date: NaiveDate, target_type: &DataType) -> Option<ScalarValue> { |
There was a problem hiding this comment.
moved to method on ScalarValue
| /// `date_trunc` floors a timestamp to the start of the given period, so | ||
| /// for any aligned literal `L` the preimage is the half-open interval | ||
| /// `[L, L + 1 granularity)`. | ||
| /// |
| /// This is local midnight unless the timezone skips midnight that day (for | ||
| /// example a daylight saving transition from 00:00 to 01:00), in which case | ||
| /// it is the first instant after the gap. If midnight occurs twice (a | ||
| /// transition back from 01:00 to 00:00) the earlier instant is returned. |
There was a problem hiding this comment.
If you want a reference for this it is the convention described in RFC5545§3.3.5
| // less than a day, so that instant is before the transition), and | ||
| // use it to convert midnight to the transition instant. | ||
| let probe = midnight.checked_sub_signed(Duration::days(1))?; | ||
| let offset_before = tz.offset_from_utc_datetime(&probe).fix(); |
There was a problem hiding this comment.
I'm pretty sure this works, but it's baffling.midnight is a local time so therefore is probe. I think there needs to be a comment saying why it is fine to use it as UTC, I think that's because we just care about some timestamp that is sufficiently before the discontinuity and it doesn't matter exactly what that time is.
There was a problem hiding this comment.
I added the comment to explain the reasoning (it was of course written by an LLM but I double checked it and clarified the comment)
Which issue does this PR close?
DATE_TRUNC(<col>) == <constant>)when pushed down #18319.Rationale for this change
Enable pruning for predicates with
date_truncon the column sideWhat changes are included in this PR?
preimageimplementation fordate_truncdate_to_scalarto a common crateAre these changes tested?
Yes, unit tests
Are there any user-facing changes?
No