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

Fixed #33037 -- Fixed Trunc() with offset timezones on MySQL, SQLite, Oracle. #17814

Merged
merged 2 commits into from Feb 15, 2024

Conversation

adzshaf
Copy link
Contributor

@adzshaf adzshaf commented Feb 3, 2024

Fixed ticket: 33037

Changes:

  • Fixing split_tzname_delta by adding :00 to offset if the offset does not have minute format.
    Example, offset that will be passed into this function is 10, but MySQL and Oracle cannot parse it since it didn't follow correct format, e.g: 10:00.
    This fix will also fix bug on SQLite. Previously, the tuple unpacking returned error since the offset will be "10" and its split result become ["10"].
  • Add fallback during parse datetime on SQLite. tzname become empty from split_tzname_delta result, so it would be error. Hence, use conn_tzname for fallback value.

I added test which is nearly identical of this part of test, with its difference on which timezone that is used. I think we can extract test functions from these parts.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

Copy link
Contributor

@LilyFoote LilyFoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

I've included some suggestions that I think would improve the tests a bit.

tests/db_functions/datetime/test_extract_trunc.py Outdated Show resolved Hide resolved
tests/db_functions/datetime/test_extract_trunc.py Outdated Show resolved Hide resolved
tests/db_functions/datetime/test_extract_trunc.py Outdated Show resolved Hide resolved
tests/db_functions/datetime/test_extract_trunc.py Outdated Show resolved Hide resolved
tests/db_functions/datetime/test_extract_trunc.py Outdated Show resolved Hide resolved
@LilyFoote
Copy link
Contributor

buildbot, test on oracle.

@felixxm felixxm changed the title Fixed #33037 -- TruncDay error when using offset timezones on MySQL, SQLite, Oracle. Fixed #33037 -- Fixed Trunc() with offset timezones on MySQL, SQLite, Oracle. Feb 5, 2024
@felixxm
Copy link
Member

felixxm commented Feb 5, 2024

@adzshaf Thanks 👍 Please take a look at @LilyFoote's test suggestions.

@LilyFoote
Copy link
Contributor

Hey @adzshaf! I think you misunderstood what I was suggesting with subTest - I meant that you could loop over the different truncation types (year, quarter, etc) and call the assert helpers in that loop.

Something like:

     truncations = ("year", "quarter", "month", "week", "day", "hour", "minute", "second")
     for truncation in truncations:
         with subTest(truncation=truncation):
             assert_datetime_to_time_kind(truncation)
             assert_datetime_kind(truncation)

@adzshaf
Copy link
Contributor Author

adzshaf commented Feb 10, 2024

@LilyFoote my bad for not confirming it. OK, I understand, but should we also do the same for test_trunc_func_with_timezone? Or we just left as it is, and do it only on my test?

@LilyFoote
Copy link
Contributor

Let's leave the other test as it was for now. It might be worth updating it too in a separate PR/commit later, but lets get the main change ready first.

@LilyFoote
Copy link
Contributor

@LilyFoote my bad for not confirming it.

It's OK - I realise I wasn't at all clear with what I meant!

@felixxm
Copy link
Member

felixxm commented Feb 10, 2024

Let's leave the other test as it was for now. It might be worth updating it too in a separate PR/commit later, but lets get the main change ready first.

Yes, we don't mix cleanups with bugfixes.

@felixxm felixxm force-pushed the ticket-33037-trunc-offset-error branch from b7488e9 to 22285d3 Compare February 15, 2024 09:04
@felixxm
Copy link
Member

felixxm commented Feb 15, 2024

@adzshaf Thanks 👍 Welcome aboard ⛵

I moved cleanups to a separate commit, made small edits, and used subTest() as Lily suggested.

@adzshaf
Copy link
Contributor Author

adzshaf commented Feb 15, 2024

@felixxm thanks a lot! Sorry for the inconvenience, haven't got time to fix it yet 🙇

@felixxm felixxm merged commit 22285d3 into django:main Feb 15, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants