Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QP local date filtering and formatting overhaul #40416

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

camsaul
Copy link
Member

@camsaul camsaul commented Mar 21, 2024

Stop doing dumb things with local dates that give you unexpected results.

  1. Format LocalDates in QP results as dates rather than (offset) date times

    Instead of returning them as "2024-03-20T00:00:00-07:00", return them as "2024-03-20".

    Also, format LocalDateTime as a local date time, LocalTime as a local time, etc. (don't add offsets to things that never had them in the first place)

  2. Bucketing on a date extraction bucket -- :day, :month, :week, :quarter, or :year -- should now return a LocalDate, rather than an OffsetDateTime. This was needed to fix the underlying issues we were running into. The FE code actually does the right thing either way, but drivers using our test suite are going to need some updates for tests to pass.

Honestly this is the way we should have been doing things all along.

Fixes #20333
(actually this might already be fixed, need to verify)

Might fix some other bugs like

@camsaul camsaul requested a review from a team March 21, 2024 00:56
@metabase-bot metabase-bot bot added the .Team/QueryProcessor :hammer_and_wrench: label Mar 21, 2024
@camsaul camsaul added the backport Automatically create PR on current release branch on merge label Mar 21, 2024
Copy link

replay-io bot commented Mar 21, 2024

Status Complete ↗︎
Commit a1b7dbe
Results
5 Failed
⚠️ 7 Flaky
2395 Passed

@camsaul camsaul changed the title QP local date filtering overhaul QP local date filtering and formatting overhaul Mar 21, 2024
@camsaul camsaul added no-backport Do not backport this PR to any branch backport Automatically create PR on current release branch on merge and removed backport Automatically create PR on current release branch on merge no-backport Do not backport this PR to any branch labels Mar 21, 2024
@camsaul
Copy link
Member Author

camsaul commented Mar 21, 2024

I'm not sure if this is going to be backportable or not, there are a lot of changes in here.

@camsaul camsaul marked this pull request as draft March 22, 2024 19:13
@camsaul camsaul added no-backport Do not backport this PR to any branch and removed backport Automatically create PR on current release branch on merge labels Mar 22, 2024
@camsaul camsaul marked this pull request as ready for review April 4, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryProcessor :hammer_and_wrench:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question Builder: Generate SQL with naive dates instead of localized timestamps
2 participants