Fix filtering on LocalDates #40530
Merged
Fix filtering on LocalDates #40530
Conversation
This file contains 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
camsaul
added
the
backport
|
camsaul
added
backport
snoe
reviewed
Mar 26, 2024
| (let [t (parse-temporal-string-literal-to-class s OffsetDateTime)] | ||
| [:absolute-datetime t target-unit])) | ||
|
|
||
| (defmethod parse-temporal-string-literal :type/DateTimeWithZoneID |
There was a problem hiding this comment.
wrap-temporal-string-literal may be a better name.
snoe
approved these changes
Mar 26, 2024
| ;; with it. | ||
| t)) | ||
|
|
||
| (defmethod coerce-temporal [LocalDate LocalTime] [_t _target-class] (throw-invalid-time)) |
There was a problem hiding this comment.
I feel that a few comment blocks explaining some of the things to consider would be helpful.
- All Local to Offset/Zoned conversions must consider
results-timezone-id - All Offset/Zoned to Local conversions do not need to consider
results-timezone-idbecause session timezone will already be applied (is that correct?) - Others?
| (defmethod coerce-temporal [LocalTime ZonedDateTime] [_t _target-class] (throw-invalid-datetime)) | ||
| (defmethod coerce-temporal [OffsetTime ZonedDateTime] [_t _target-class] (throw-invalid-datetime)) | ||
| (defmethod coerce-temporal [LocalDateTime ZonedDateTime] [t _target-class] (t/zoned-date-time t (t/zone-id (qp.timezone/results-timezone-id)))) | ||
| (defmethod coerce-temporal [OffsetDateTime ZonedDateTime] [t _target-class] t) ; OffsetDateTime is perfectly fine. |
There was a problem hiding this comment.
I'm not sure it is perfectly fine, I believe adding/comparing time to an offsetdatetime across a daylight savings shift could give an incorrect result. I'm not sure when we would actually get into this conversion though to test this scenario.
snoe> snoe=# set timezone = "America/Edmonton";
SET
snoe> snoe=# select ('2024-03-09T23:59'::timestamp) at time zone 'America/Chicago' + '3 hour'::interval;
?column?
------------------------
2024-03-10 01:59:00-07
(1 row)
snoe> snoe=# select ('2024-03-09T23:59'::timestamp) at time zone 'America/Chicago' + '4 hour'::interval;
?column?
------------------------
2024-03-10 03:59:00-06
(1 row)
snoe> snoe=# set timezone = "MST";
SET
snoe> snoe=# select ('2024-03-09T23:59'::timestamp) at time zone 'America/Chicago' + '3 hour'::interval;
?column?
------------------------
2024-03-10 01:59:00-07
(1 row)
snoe> snoe=# select ('2024-03-09T23:59'::timestamp) at time zone 'America/Chicago' + '4 hour'::interval;
?column?
------------------------
2024-03-10 02:59:00-07
(1 row)
|
@camsaul Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
camsaul
added a commit
that referenced
this pull request
Mar 27, 2024
* Split from #40146: only the filter changes * Don't include format-rows changes here. * Test fixes 🔧 * Test fix 🔧 * Misc test fixes 🔧 * Test fix 🔧 * Tweak SQL Server fix
camsaul
added a commit
that referenced
this pull request
Mar 27, 2024
* Add backport resolution script * Fix filtering on LocalDates (#40530) * Split from #40146: only the filter changes * Don't include format-rows changes here. * Test fixes 🔧 * Test fix 🔧 * Misc test fixes 🔧 * Test fix 🔧 * Tweak SQL Server fix * Fix Kondo errors * Updated api/dataset tests * Test fix 🔧 --------- Co-authored-by: Metabase bot <metabase-bot@metabase.com> Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com> Co-authored-by: Cam Saul <github@camsaul.com>
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.
Stop doing dumb things with local dates that give you unexpected results.
Don't parse a local date literal like
2024-03-20to anOffsetDateTimeorZonedDateTime. Previously, we'd try to parse it to something like#t "2024-03-20T00:00:00-07:00[US/Pacific]"using the QP results timezone (eitherreport-timezoneif the DB supports session timezones, or the database timezone if it does not). Predictably, this caused bugs. Just parse it to aLocalDateinstead.Don't try to optimize a
LocalDate = LocalDateorday(LocalDate) = day(LocalDate)filters to:betweenfilters; we can leave them as=. E.g. instead of generatingwe should just generate
This causes way less problems and is more performant, and is nicer to look at.
Fixes #39769
Fixes #40332
This was split off from #40416, which is a more significant overhaul that fixes more things, but too big to backport.