Skip to content

issue 1166 Update test to use SQLAlchemy 2.0 compatible select statement #1167

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

Merged
merged 9 commits into from
Mar 13, 2025

Conversation

shirzady1934
Copy link
Contributor

This PR resolves #1166 by update test to use SQLAlchemy 2.0 compatible select statement

Copy link
Contributor

@loicdiridollou loicdiridollou left a comment

Choose a reason for hiding this comment

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

One small change to make sure we are testing the right thing.

tests/test_io.py Outdated
pd.read_sql(
session.query(Temp.quantity).statement, session.connection()
)
stmt = sqlalchemy.select(Temp.quantity)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you are modifying the type of stmt so you are not really testing what used to be tested. What is needed is to update the stubs so that this test passes.
Without changing this test, what needs to be done is to update a pyi file.
For that I would recommend you look into what is the new type of stmt with the update of sqlalchemy, you will see that stmt has now an extra possible type UpdateBase and what how that impacts the stubs in pandas-stubs/io/sql.pyi. There you see that the Type Alias _SQLStatement is too restrictive and you need to allow UpdateBase in that TypeAlias too.
Feel free to let me know if something is unclear.

@shirzady1934
Copy link
Contributor Author

@loicdiridollou I reverted my previous commits and added extra possible type UpdateBase to _SQLStatement, is that ok?

@@ -33,6 +34,7 @@ _SQLStatement: TypeAlias = (
| sqlalchemy.sql.expression.TextClause
| sqlalchemy.sql.Select
| FromStatement
| UpdateBase
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of importing UpdateBase, use the pattern as in lines 33 and 34, and place sqlalchemy.sql.expression.UpdateBase here

@shirzady1934
Copy link
Contributor Author

@Dr-Irv Applied the change

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

while you were working on this, we put a constraint in pyproject.toml on sqlalchemy so that our CI would pass.

Can you change this line in that file:

SQLAlchemy = ">=2.0.12,<2.0.39"

to

SQLAlchemy = ">=2.0.39"

Co-authored-by: Irv Lustig <[email protected]>
Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

thanks @shirzady1934

@Dr-Irv Dr-Irv merged commit 7328e89 into pandas-dev:main Mar 13, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SqlAlchemy update to 2.0.39 breaks read_sql
3 participants