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

refactor: deprecate usage of cursor.execute statements in favor of the in class implementation of execute. #60748

Merged
merged 7 commits into from
Feb 12, 2025
Merged

refactor: deprecate usage of cursor.execute statements in favor of the in class implementation of execute. #60748

merged 7 commits into from
Feb 12, 2025

Conversation

gmcrocetti
Copy link
Contributor

@gmcrocetti gmcrocetti commented Jan 21, 2025 •
edited
Loading

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@gmcrocetti gmcrocetti force-pushed the refactor-io-sql-execute branch 5 times, most recently from 04fee59 to e9cbf63 Compare January 22, 2025 02:15
@gmcrocetti
Copy link
Contributor Author

gmcrocetti commented Jan 22, 2025 •
edited
Loading

Hello @WillAyd.
So this is the follow up of #60376.
I updated the code base to use panda's execute implementation as much as possible. I couldn't replace all places and a simple git grep '.exec' -- pandas/io/sql.py will show you that. Anyways what in my opinion is worth mentioning and asking for a review is in the following:

  1. SQLTable._execute_insert
  2. SQLTable._execute_insert_multi
  3. SQLiteTable._execute_insert
  4. SQLiteTable._execute_insert_multi

This should be no problem because we can always wrap that execution around a try-except block (as I did in SQLiteTable._execute_insert)

Would you mind taking a look and LMK what you think while in draft ?

@gmcrocetti gmcrocetti force-pushed the refactor-io-sql-execute branch from e9cbf63 to ff41294 Compare January 22, 2025 02:16
WillAyd
WillAyd reviewed Jan 22, 2025
View reviewed changes
@WillAyd WillAyd added Refactor Internal refactoring of code IO SQL to_sql, read_sql, read_sql_query labels Jan 22, 2025
@gmcrocetti gmcrocetti force-pushed the refactor-io-sql-execute branch 2 times, most recently from 9e0f436 to 804fb3e Compare January 22, 2025 23:55
@gmcrocetti gmcrocetti requested a review from WillAyd January 22, 2025 23:55
@gmcrocetti gmcrocetti mentioned this pull request Jan 23, 2025
Merged
5 tasks
WillAyd
WillAyd requested changes Jan 27, 2025
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

One comment but generally this looks good. @mroeschke can you take a look as well?

@gmcrocetti gmcrocetti marked this pull request as ready for review February 1, 2025 23:18
@gmcrocetti gmcrocetti marked this pull request as draft February 1, 2025 23:18
WillAyd
WillAyd requested changes Feb 10, 2025
View reviewed changes
@gmcrocetti gmcrocetti force-pushed the refactor-io-sql-execute branch 2 times, most recently from ab1b946 to 71e0d24 Compare February 10, 2025 18:23
@gmcrocetti gmcrocetti requested a review from WillAyd February 10, 2025 19:25
@gmcrocetti
Copy link
Contributor Author

Hello @WillAyd ,

All tests passed here and therefore I'm opening this PR to review. I have updated the documentation :).

@gmcrocetti gmcrocetti marked this pull request as ready for review February 11, 2025 23:46
WillAyd
WillAyd reviewed Feb 12, 2025
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Small comment but otherwise this lgtm. @mroeschke do you mind taking a look as well?

@gmcrocetti gmcrocetti requested a review from WillAyd February 12, 2025 11:25
mroeschke
mroeschke reviewed Feb 12, 2025
View reviewed changes
@mroeschke mroeschke added this to the 3.0 milestone Feb 12, 2025
mroeschke
mroeschke reviewed Feb 12, 2025
View reviewed changes
@mroeschke
Copy link
Member

Looks like this PR needs a rebase with the main branch

@gmcrocetti gmcrocetti force-pushed the refactor-io-sql-execute branch from 17903e2 to 8b7241a Compare February 12, 2025 19:03
@gmcrocetti gmcrocetti requested a review from mroeschke February 12, 2025 19:10
@mroeschke mroeschke merged commit b601a0c into pandas-dev:main Feb 12, 2025
42 checks passed
@mroeschke
Copy link
Member

Thanks @gmcrocetti

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

@MarcoGorelli MarcoGorelli Awaiting requested review from MarcoGorelli

@datapythonista datapythonista Awaiting requested review from datapythonista

@WillAyd WillAyd Awaiting requested review from WillAyd

Assignees
No one assigned
Labels
IO SQL to_sql, read_sql, read_sql_query Refactor Internal refactoring of code
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@gmcrocetti @mroeschke @WillAyd