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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #22114 +/- ##
==========================================
- Coverage 80.72% 80.70% -0.02%
==========================================
Files 1089 1089
Lines 368911 369190 +279
Branches 368911 369190 +279
==========================================
+ Hits 297785 297967 +182
- Misses 53374 53439 +65
- Partials 17752 17784 +32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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