Skip to content

Conversation

@guan404ming
Copy link
Member

@guan404ming guan404ming commented May 2, 2025

Related Issue

Why

get_pandas_df deprecated in #48875, which would be replaced by get_df. Thus, we need to migrate them in providers.

How

  • This PR is focus on migration of Amazon/transfer
  • Migrate unit test
  • Refine the common-sql type

Note

There is some workaround for type here. It should be removed after the common-sql bundle the refined type definition. I've added the TODO in the comment and would get back here after the release.


^ 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.

@guan404ming guan404ming force-pushed the migrate-amazon-transfer branch 2 times, most recently from 86d526e to 8a30a7f Compare May 2, 2025 17:53
@guan404ming guan404ming marked this pull request as ready for review May 2, 2025 18:22
@guan404ming guan404ming force-pushed the migrate-amazon-transfer branch 2 times, most recently from 51667c8 to bc63964 Compare May 3, 2025 17:06
@guan404ming guan404ming force-pushed the migrate-amazon-transfer branch from 98da798 to 49c53c5 Compare May 5, 2025 10:00
@guan404ming
Copy link
Member Author

guan404ming commented May 5, 2025

Failed since the hive also need the overload like something I added in common-sql. I would open PR to solve it later.

@guan404ming
Copy link
Member Author

Here is the PR #50211

@guan404ming
Copy link
Member Author

guan404ming commented May 6, 2025

Hi, I’ve tried to figure out why mypy passes locally but fails in CI: the CI pipeline seems not import (or install ?) the dependency (common-sql/sql.pyi) mypy needs for its checks. If you compare the local and CI error logs, you’ll see that with our new type overload the error disappears locally, but CI still reports it. I'm not sure that should we update the CI configuration to force import that file, or just add a # type: ignore here or is there any better way to solve this in Slack/transfer and this one?

Thanks in advance!

Local:
image

CI:
image

@guan404ming
Copy link
Member Author

guan404ming commented May 6, 2025

When I keep searching the answer I find the type def did not really following the newest python typing spec, thus I update them in #50229. I think maybe it could solve the error here since it didn't rely on the pyi file but to directly write the overload in the .py which I think it must be imported here.

@potiuk potiuk force-pushed the migrate-amazon-transfer branch from 49c53c5 to 814d079 Compare May 6, 2025 14:09
@guan404ming
Copy link
Member Author

guan404ming commented May 6, 2025

Mypy seems passed and even the Hive related error is passed. It seems like related to the change here #50198 right?

@guan404ming guan404ming force-pushed the migrate-amazon-transfer branch from 814d079 to 8ed059e Compare May 6, 2025 14:51
@guan404ming
Copy link
Member Author

Pushed again to ensure it is stable.

@guan404ming guan404ming force-pushed the migrate-amazon-transfer branch from 8ed059e to 47bb0cf Compare May 6, 2025 14:53
@guan404ming
Copy link
Member Author

Mypy works great and push again for unrelated ci error

Co-authored-by: Jarek Potiuk <potiuk@apache.org>
@guan404ming guan404ming force-pushed the migrate-amazon-transfer branch from 47bb0cf to c40b4be Compare May 6, 2025 16:38
@eladkal eladkal changed the title Migrate Amazon/transfer to use get_df Migrate HiveToDynamoDBOperator and SqlToS3Operator to use get_df May 8, 2025
@eladkal eladkal merged commit 35186bc into apache:main May 8, 2025
68 checks passed
@guan404ming guan404ming deleted the migrate-amazon-transfer branch May 8, 2025 07:13
@guan404ming
Copy link
Member Author

Thanks for the suggestions and review!

ayush3singh pushed a commit to ayush3singh/airflow that referenced this pull request May 8, 2025
github-actions bot pushed a commit that referenced this pull request Aug 12, 2025
… use `get_df` (#50126)

(cherry picked from commit 35186bc)

Co-authored-by: Guan Ming(Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
Co-authored-by: Jarek Potiuk <potiuk@apache.org>
@github-actions
Copy link

Backport successfully created: v3-0-test

Status Branch Result
v3-0-test PR Link

ashb pushed a commit that referenced this pull request Aug 12, 2025
… use `get_df` (#50126) (#54409)

* [v3-0-test] Migrate `HiveToDynamoDBOperator` and `SqlToS3Operator` to use `get_df` (#50126)
(cherry picked from commit 35186bc)

Co-authored-by: Guan Ming(Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
Co-authored-by: Jarek Potiuk <potiuk@apache.org>
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.

3 participants