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.

Sign up for GitHub

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

Jump to bottom

BUG: Make io.sql.execute raise TypeError on Engine or URI string. #50177

Merged
merged 1 commit into from
Dec 15, 2022
Merged

BUG: Make io.sql.execute raise TypeError on Engine or URI string. #50177

merged 1 commit into from
Dec 15, 2022

Conversation

cdcadman
Copy link
Contributor

@cdcadman cdcadman commented Dec 10, 2022 •
edited by mroeschke
Loading

phofl
phofl reviewed Dec 11, 2022
View reviewed changes
ci/deps/actions-38-minimum_versions.yaml Outdated
@@ -51,7 +51,7 @@ dependencies:
- pyxlsb=1.0.8
- s3fs=2021.08.0
- scipy=1.7.1
- sqlalchemy=1.4.16
- sqlalchemy=1.4.45
Copy link
Member

Choose a reason for hiding this comment

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

I am -1 on this. In general, we want to support versions that are up to a year old. This is the newest release and too strict

phofl
phofl reviewed Dec 11, 2022
View reviewed changes
Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

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

Can you please open an issue describing the problem you are fixing?

@cdcadman
Copy link
Contributor Author

This bug does not exist in any release of pandas. It only exists in the main branch due to a PR that I created. So this version restriction would only show up in pandas 2.0. I'm not able to open a bug report issue, because the template requires me to check that this bug exists on the latest version of pandas. I discovered the bug while I was trying to complete #48576. The pandas.io.sql.execute method is not well-documented. The only mentioned I found is above this line: https://pandas.pydata.org/docs/user_guide/io.html?highlight=sql%20execute#engine-connection-examples. If it's ok to change the method so that it doesn't allow either an Engine or a str to be passed as the connectable, then the bug could be fixed that way instead of by using the Result.freeze method of sqlalchemy, which had a bug in it until version 1.4.45.

@phofl
Copy link
Member

phofl commented Dec 11, 2022 •
edited
Loading

You can open an issue without using the template at the bottom of that page.

2.0 will come out early next year, the limit is still to strict for us. We updated the minimum versions a couple of months ago.

Can we revert your initial pr if this can not get fixed otherwise?

@cdcadman
Copy link
Contributor Author

If you need to revert PR #49531, then you will also need to revert PR #49757. I think this issue needs to be resolved in order for pandas to support sqlalchemy 2.0. I will open a general issue.

@cdcadman cdcadman marked this pull request as draft December 11, 2022 16:20
@phofl
Copy link
Member

phofl commented Dec 11, 2022

Yep please open an issue with a reproducible example. Also please link the pr that caused this there, makes investigating easier.

@cdcadman
Copy link
Contributor Author

I have opened an issue: #50185

@cdcadman cdcadman marked this pull request as ready for review December 12, 2022 10:56
@cdcadman cdcadman changed the title BUG: ensure connection does not close before retrieving results from io.sql.execute. CLN: deprecate io.sql.execute and raise TypeError on Engine or URI string Dec 12, 2022
phofl
phofl reviewed Dec 12, 2022
View reviewed changes
pandas/io/sql.py Outdated
@@ -180,6 +178,15 @@ def execute(sql, con, params=None):
-------
Results Iterable
"""
warnings.warn(
"pandas.io.sql.execute will be removed in pandas 3.0",
DeprecationWarning,
Copy link
Member

Choose a reason for hiding this comment

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

We want to raise a FutureWarning here.

Could you remove the deprecation for now? We want to finish enforcing old deprecations before we add new ones. Would be better to separate this from the other changes here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, do you want me to leave it in the user guide, but just change it so that it doesn't imply you can pass an Engine? I've updated the PR this way, and removed the warning (or did you want me to change the warning to FutureWarning in this PR?)

Copy link
Member

Choose a reason for hiding this comment

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

No removing the warning is fine, the FutureWarning comment was for the future.

I am ok with deleting it from the user guide immediately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it is removed from the user guide now.

@cdcadman cdcadman changed the title CLN: deprecate io.sql.execute and raise TypeError on Engine or URI string BUG: Make io.sql.execute raise TypeError on Engine or URI string. Dec 12, 2022
phofl
phofl approved these changes Dec 13, 2022
View reviewed changes
Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

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

lgtm

@cdcadman cdcadman mentioned this pull request Dec 13, 2022
Merged
5 tasks
@cdcadman
Copy link
Contributor Author

@phofl thanks for approving. Is it ok to merge? I don't have write access.

@phofl
Copy link
Member

phofl commented Dec 15, 2022

@mroeschke could you have a quick look?

@mroeschke mroeschke added the IO SQL to_sql, read_sql, read_sql_query label Dec 15, 2022
@mroeschke mroeschke added this to the 2.0 milestone Dec 15, 2022
mroeschke
mroeschke approved these changes Dec 15, 2022
View reviewed changes
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

LGTM. Will repurprose the referenced issue to deprecate io.sql.execute in a follow up PR

@mroeschke mroeschke merged commit a8c2000 into pandas-dev:main Dec 15, 2022
@mroeschke
Copy link
Member

Thanks @cdcadman

@cdcadman cdcadman deleted the execute_bug branch December 15, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroeschke mroeschke mroeschke approved these changes

@phofl phofl phofl approved these changes

Assignees
No one assigned
Labels
IO SQL to_sql, read_sql, read_sql_query
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cdcadman @phofl @mroeschke