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 #33355 -- Optimized SQLite backend connection functions. #15173

Merged
merged 4 commits into from Dec 23, 2021

Conversation

adamchainz
Copy link
Sponsor Member

Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

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

Boo! 😄 But, yes, while less elegant, this makes a lot of sense.

It's straightforward. I only had a few minor comments around naming.

A few more thoughts:

  • It might be worth moving all these _sqlite_* functions into a separate module to decruft base.py, maybe a private django.db.backends.sqlite._functions module?
  • If in a separate module, we could do from math import ... and remove the attribute lookup in each function for another little speed boost?
  • Maybe that module could also contain a register(connection) function so we can move all of the registration out of base.py?

django/db/backends/sqlite3/base.py Outdated Show resolved Hide resolved
django/db/backends/sqlite3/base.py Outdated Show resolved Hide resolved
django/db/backends/sqlite3/base.py Outdated Show resolved Hide resolved
@adamchainz
Copy link
Sponsor Member Author

It might be worth moving all these _sqlite_* functions into a separate module to decruft base.py, maybe a private django.db.backends.sqlite._functions module?

Yes I will give that a try.

If in a separate module, we could do from math import ... and remove the attribute lookup in each function for another little speed boost?

And that!

Maybe that module could also contain a register(connection) function so we can move all of the registration out of base.py?

Yes.

Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

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

A few follow on comments...

...and then, as this issue is about optimising these functions, I couldn't resist doing a more thorough review of these as they're being moved around anyway. Seems like a good opportunity to eke out some more improvements.

Aside from the comments below, could you also group the _sqlite_*_extract() functions together, and the _sqlite_*_trunc(), etc., etc.

django/db/backends/sqlite3/_functions.py Outdated Show resolved Hide resolved
django/db/backends/sqlite3/_functions.py Outdated Show resolved Hide resolved
django/db/backends/sqlite3/_functions.py Show resolved Hide resolved
django/db/backends/sqlite3/_functions.py Outdated Show resolved Hide resolved
django/db/backends/sqlite3/_functions.py Outdated Show resolved Hide resolved
django/db/backends/sqlite3/_functions.py Outdated Show resolved Hide resolved
django/db/backends/sqlite3/_functions.py Outdated Show resolved Hide resolved
django/db/backends/sqlite3/_functions.py Show resolved Hide resolved
django/db/backends/sqlite3/_functions.py Show resolved Hide resolved
django/db/backends/sqlite3/_functions.py Show resolved Hide resolved
Copy link
Sponsor Member Author

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

I extracted the changes to the trunc functions with the bug fix into #15182 , which should be merged first. Then I will rebuild this PR as a new series of commits with fewer steps.

@ngnpope
Copy link
Member

ngnpope commented Dec 16, 2021

@adamchainz Could you rebase this one now that #15182 has landed? Then I'll give it another look through.

@adamchainz
Copy link
Sponsor Member Author

I've rebuilt the branch as a new series of commits. I think I managed to capture everything.

Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

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

Thanks Adam, this looks great! 🌟

Also thanks for humouring me with looking through for little tweaks. When shifting a bunch of stuff around like this it's worth making sure we get the maximum benefit while we're in the area.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@adamchainz Thanks 👍

I merged the first two commits in a8fa3e5 and c4328c2.

django/db/backends/sqlite3/_functions.py Outdated Show resolved Hide resolved
django/db/backends/sqlite3/_functions.py Outdated Show resolved Hide resolved
except (ValueError, TypeError):
return None
if connector == '+':
# typecast_timestamp returns a date or a datetime without timezone.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# typecast_timestamp returns a date or a datetime without timezone.
# typecast_timestamp() returns a date or a datetime without timezone.

django/db/backends/sqlite3/_functions.py Outdated Show resolved Hide resolved
tests/expressions/tests.py Outdated Show resolved Hide resolved
@felixxm
Copy link
Member

felixxm commented Dec 23, 2021

I merged LPad() optimization in fa4b2c1.

@adamchainz adamchainz force-pushed the ticket_33355 branch 2 times, most recently from 5927599 to 60fc594 Compare December 23, 2021 09:50
adamchainz and others added 3 commits December 23, 2021 11:47
Co-Authored-By: Nick Pope <nick@nickpope.me.uk>
…ps on SQLite.

Co-Authored-By: Nick Pope <nick@nickpope.me.uk>
Co-Authored-By: Nick Pope <nick@nickpope.me.uk>
@felixxm
Copy link
Member

felixxm commented Dec 23, 2021

Thanks both 🥇

I reorganized commits.

Copy link
Sponsor Member Author

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

Okay, looks good!

@felixxm felixxm merged commit 5f6a727 into django:main Dec 23, 2021
@adamchainz
Copy link
Sponsor Member Author

Yyyyyyeah

@adamchainz adamchainz deleted the ticket_33355 branch December 23, 2021 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants