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

Fix SQL parameter type #757

Merged
merged 2 commits into from
Jul 25, 2023
Merged

Fix SQL parameter type #757

merged 2 commits into from
Jul 25, 2023

Conversation

Tolker-KU
Copy link
Contributor

Align parameter type to SQL functions: https://github.com/pandas-dev/pandas/blob/main/pandas/io/sql.py#L370

  • Closes #xxxx (Replace xxxx with the Github issue number)
  • Tests added: Please use assert_type() to assert the type of any return value

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.

Can you add a test that illustrates why this is needed, i.e., why the current types for the params argument don't work?

@Tolker-KU
Copy link
Contributor Author

Hi!

Thanks for getting back quickly! I'm not quite sure how to test this change. Eventually we are calling into SQLAlchemy when using these methods - and SQLAlchemy types the parameters as Any. This is the reasoning behind the change

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jul 24, 2023

Hi!

Thanks for getting back quickly! I'm not quite sure how to test this change. Eventually we are calling into SQLAlchemy when using these methods - and SQLAlchemy types the parameters as Any. This is the reasoning behind the change

If you look at tests/test_io.py, and search for read_sql_query(), you will want to add a test similar to one of those, using example code that currently fails with the stubs, but would work with your 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.

In thinking about this, I think the Any type is too wide. For example, you couldn't pass an instance of any class as a parameter to an SQL call, it would have to be some kind of scalar. So can you change the places where you used Any in this PR to use Scalar instead, which you can import from pandas._typing ?

@Tolker-KU
Copy link
Contributor Author

Sounds like a good solution to me. I've made 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.

Thanks @Tolker-KU

@Dr-Irv Dr-Irv merged commit 5a9abdd into pandas-dev:main Jul 25, 2023
13 checks passed
@Tolker-KU Tolker-KU deleted the sql_params branch July 26, 2023 05:17
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.

None yet

2 participants