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

[v5.1.0] Refactor the rest of the repositories to use sqlalchemy core 1.4 #632

Merged
merged 10 commits into from
Feb 26, 2024

Conversation

cmyui
Copy link
Member

@cmyui cmyui commented Feb 19, 2024

Co-authored-by: James Wilson tsunyoku@users.noreply.github.com

Describe your changes

Related Issues / Projects

Resolves #394

Checklist

  • I've manually tested my code

@tsunyoku tsunyoku added performance Improvements to resource usage without changing functionality code quality Something is implemented poorly labels Feb 19, 2024
@tsunyoku tsunyoku self-requested a review February 20, 2024 00:05
@tsunyoku
Copy link
Contributor

@cmyui should we do alembic stuff in this PR or another one? if the latter, i want to merge this into master

@tsunyoku
Copy link
Contributor

tsunyoku commented Feb 20, 2024

also, i was thinking we could create a database adapter that wraps around databases.Database to take a ClauseElement and we do the query compilation stuff there so that we're not doing compiled = stmt.compile(dialect=DIALECT), str(compiled) and compiled.params everywhere

@cmyui
Copy link
Member Author

cmyui commented Feb 20, 2024

should we do alembic stuff in this PR or another one? if the latter, i want to merge this into master

I'm confident there are quite a few bugs in this so I'd like to diff against the generated alembic migrations first. I think a new alembic branch can be based on this; that way we can treat them separately, and merge this without necessarily merging the alembic work. Easy to change alembics base to master if we merge this (in fact, I think github may do so automatically).

@cmyui
Copy link
Member Author

cmyui commented Feb 20, 2024

also, i was thinking we could create a database adapter that wraps around databases.Database to take a ClauseElement and we do the query compilation stuff there so that we're not doing compiled = stmt.compile(dialect=DIALECT), str(compiled) and compiled.params everywhere

Yup, that's a good idea. It seemed very error prone since stmt alone will be accepted by the db functions -- very easy to miss one.

@tsunyoku
Copy link
Contributor

tsunyoku commented Feb 20, 2024

sorry for the loaded commit - was unavoidable. latest commit adds a wrapper around databases as discussed above, and additionally ensures all of our queries are compatible with sqlalchemy 2.0 (all of the selects needed a slight syntax change)

squashed quite a few bugs in the process; there were a couple random ones but the majority were the sqlalchemy statements being incorrectly passed to the database

in the future i want to refactor the database wrapper to be strict and only accept ClauseElement rather than ClauseElement | str however until every query is using sqlalchemy this isn't possible.

@cmyui cmyui marked this pull request as ready for review February 26, 2024 01:57
@cmyui
Copy link
Member Author

cmyui commented Feb 26, 2024

Alright did my most thorough manual testing in a while on this PR & I think I've caught the major bugs. It still likely has some diffs in terms of the existing db schema vs. what this would generate, but I don't think anything major -- just things like incorrect server_default. I'll tackle those in an upcoming PR, but from a runtime perspective this looks to be working as expected for all major flows (score fetch, submit, multiplayer, spectator, etc.) and commands.

@cmyui cmyui changed the title Refactor the rest of the repositories to use sqlalchemy core 1.4 [v5.1.0] Refactor the rest of the repositories to use sqlalchemy core 1.4 Feb 26, 2024
@cmyui
Copy link
Member Author

cmyui commented Feb 26, 2024

Ty for your support on this PR @tsunyoku!! Much appreciated 🎉🎉🎉

@cmyui cmyui merged commit 26b8595 into master Feb 26, 2024
5 checks passed
@cmyui cmyui deleted the more-sqlalchemy branch February 26, 2024 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Something is implemented poorly performance Improvements to resource usage without changing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: COALESCE function causes slow query
2 participants