Skip to content

Fix: ensure get_df uses SQLAlchemy engine to avoid pandas warning #52224

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

daesung9511
Copy link



Fix PostgresHook.get_df() warning when using non-SQLAlchemy connections

This PR updates the PostgresHook.get_df() implementation to avoid triggering a UserWarning from pandas when the underlying connection is not an instance of sqlalchemy.engine.Connection. The patch ensures better compatibility with non-SQLAlchemy connections and avoids log clutter during DAG execution.

This change only affects the Postgres provider and does not alter the interface or behavior for existing users using SQLAlchemy-compatible connections.

closes: #52142


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

Copy link

boring-cyborg bot commented Jun 25, 2025

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://s.apache.org/airflow-slack

Copy link
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your PR! Could you please also include unit tests?

@bugraoz93
Copy link
Contributor

Thanks for the updates!

@daesung9511
Copy link
Author

Hi, just a kind reminder for the review. I’m here and ready to hear your advice and revise the code. Thanks!
@bugraoz93

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Nice!

@potiuk
Copy link
Member

potiuk commented Jul 10, 2025

Let's see if CI agrees :)

@bugraoz93
Copy link
Contributor

Hi, just a kind reminder for the review. I’m here and ready to hear your advice and revise the code. Thanks!
@bugraoz93

Thanks for the changes!

@bugraoz93
Copy link
Contributor

I think CI needs some love on tests :)

@daesung9511
Copy link
Author

daesung9511 commented Jul 14, 2025

Hi, I modified the pyproject.toml and rolled back the code to ensure it passes with the minimal required dependencies and aligns the method signature with its superclass, as required by mypy in the unit tests.
Please let me know if anything should be further modified or removed. Otherwise, it looks like the CI has passed.
Thanks again for your time and review! @bugraoz93 @potiuk

It passed in the previous commit (133408b).

@bugraoz93
Copy link
Contributor

Thanks a lot @daesung9511! Could you please rebase the branch for a cleaner commit history? I believe it is always merged with the incoming changes. I think we can merge it afterwards. Great work!

@daesung9511 daesung9511 force-pushed the fix/postgres-hook-df-warning branch 2 times, most recently from 2deb3b3 to e86cd2a Compare July 15, 2025 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_df UserWarning for PostgresHook
3 participants