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

Move all SQL classes to common-sql provider #24836

Merged
merged 1 commit into from
Jul 6, 2022
Merged

Move all SQL classes to common-sql provider #24836

merged 1 commit into from
Jul 6, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jul 5, 2022

The DBApiHook, SQLSensor are now part of the common.sql provider.

^ 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, in newsfragments.

@boring-cyborg boring-cyborg bot added area:core-operators Operators, Sensors and hooks within Core Airflow area:dev-tools area:providers provider:Apache provider:amazon-aws AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Jul 5, 2022
@potiuk potiuk force-pushed the move-db-classes-to-common-sql-provider branch from 0024545 to 8fc883d Compare July 5, 2022 01:37
@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2022

cc: @denimalpaca - this one implements more complete version of #24790 (some tests were failiing) - it also moves the DBApi and SQLSensor to rhe new provider.

@potiuk potiuk force-pushed the move-db-classes-to-common-sql-provider branch 2 times, most recently from afa4106 to 7d35f5d Compare July 5, 2022 01:53
eladkal
eladkal reviewed Jul 5, 2022
View reviewed changes
Copy link
Contributor

@eladkal eladkal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There should be changes to pandas extra? (as this extra was created for DBApiHook)
The common.sql provider should be depended on the pandas extra isn't it?

airflow/providers/common/sql/provider.yaml Show resolved Hide resolved
@potiuk potiuk force-pushed the move-db-classes-to-common-sql-provider branch 2 times, most recently from 454d756 to 6ae722e Compare July 5, 2022 10:09
@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2022

There should be changes to pandas extra? (as this extra was created for DBApiHook)
The common.sql provider should be depended on the pandas extra isn't it?

I think extra must stay (this is optional and backwards-compatibility if it is changed/removed). But yeah - common-sql provider should depend on pandas directly. Good point.

@potiuk potiuk force-pushed the move-db-classes-to-common-sql-provider branch from 6ae722e to 6b2b5ab Compare July 5, 2022 10:13
@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2022

Added pandas to common-sql as dependency

@potiuk potiuk force-pushed the move-db-classes-to-common-sql-provider branch from 6b2b5ab to ffbffbd Compare July 5, 2022 10:20
@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2022

I also removed the "try/import" around pandas import in "DBApiHook now - because common.sql has now pandas as "hard" dependency (not an extra), so it will be installed for sure.

@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2022

That made me think... maybe we indeed want to contiue using "pandas" as extras even for common-sql ? Common-sql is now "dependency" of airlfow, and it makes panda transitive but non-optional dependency of airlfow. I think I will revert the try/import change and turn pandas on comon-sql into extra.

@potiuk potiuk force-pushed the move-db-classes-to-common-sql-provider branch from ffbffbd to 9c2043a Compare July 5, 2022 10:34
@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2022

Yeah. much better. I turned pandas into common-sql[pandas] extra

@potiuk potiuk force-pushed the move-db-classes-to-common-sql-provider branch from 9c2043a to dda2ebd Compare July 5, 2022 12:15
@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2022 •
edited
Loading

This one should be green :)

@ashb
Copy link
Member

ashb commented Jul 5, 2022 •
edited
Loading

I also removed the "try/import" around pandas import in "DBApiHook now - because common.sql has now pandas as "hard" dependency (not an extra), so it will be installed for sure.

Is that a good idea? The majority of the code (and operators) don't make use of Pandas.

Edit: Oh mis-read the conversation. Yes to keeping it as an optional extra.

@potiuk potiuk requested a review from jedcunningham July 6, 2022 18:14
@potiuk potiuk force-pushed the move-db-classes-to-common-sql-provider branch from b57b8cf to 51314de Compare July 6, 2022 18:15
@potiuk potiuk force-pushed the move-db-classes-to-common-sql-provider branch 2 times, most recently from 4becab9 to 02c4a82 Compare July 6, 2022 20:59
@potiuk
Move all SQL classes to common-sql provider
ecb4b49 
The DBApiHook, SQLSensor are now part of the common.sql provider.
@potiuk potiuk force-pushed the move-db-classes-to-common-sql-provider branch from 02c4a82 to ecb4b49 Compare July 6, 2022 21:01
@potiuk potiuk merged commit 46bbfda into apache:main Jul 6, 2022
@potiuk potiuk deleted the move-db-classes-to-common-sql-provider branch July 6, 2022 22:02
@potiuk potiuk mentioned this pull request Jul 13, 2022
Closed
95 tasks
potiuk added a commit to potiuk/airflow that referenced this pull request Jul 16, 2022
@potiuk
The 'depdendencies.json' file is removed
f34bcb6 
The dependencies.json was replaced by provider_dependencies.json in
generated folder in apache#24836 but it missed deletion of the
original file. This PR removes the old file.
@potiuk potiuk mentioned this pull request Jul 16, 2022
Merged
potiuk added a commit that referenced this pull request Jul 16, 2022
@potiuk
The 'depdendencies.json' file is removed (#25108)
8450285 
The dependencies.json was replaced by provider_dependencies.json in
generated folder in #24836 but it missed deletion of the
original file. This PR removes the old file.
@pankajastro pankajastro mentioned this pull request Jul 23, 2022
Closed
2 tasks
This was referenced Jul 25, 2022
Merged
Closed
potiuk added a commit to potiuk/airflow that referenced this pull request Jul 27, 2022
@potiuk
Move all "old" SQL operators to common.sql providers
d7b1860 
Previously, in apache#24836 we moved Hooks and added some new operators to the
common.sql package. Now we are salso moving the operators
and sensors to common.sql.
@potiuk potiuk mentioned this pull request Jul 27, 2022
Merged
@potiuk potiuk mentioned this pull request Jul 28, 2022
Merged
potiuk added a commit that referenced this pull request Jul 28, 2022
@potiuk
Move all "old" SQL operators to common.sql providers
fcaf311 
Previously, in #24836 we moved Hooks and added some new operators to the
common.sql package. Now we are salso moving the operators
and sensors to common.sql.
potiuk added a commit that referenced this pull request Jul 28, 2022
@potiuk
Move all "old" SQL operators to common.sql providers
af317a4 
Previously, in #24836 we moved Hooks and added some new operators to the
common.sql package. Now we are salso moving the operators
and sensors to common.sql.
potiuk added a commit that referenced this pull request Jul 29, 2022
@potiuk
Move all "old" SQL operators to common.sql providers (#25350)
acab8f5 
Previously, in #24836 we moved Hooks and added some new operators to the
common.sql package. Now we are salso moving the operators
and sensors to common.sql.
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Aug 12, 2022
@was-av was-av mentioned this pull request Oct 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedcunningham jedcunningham jedcunningham approved these changes

@josh-fell josh-fell josh-fell approved these changes

@eladkal eladkal eladkal approved these changes

@turbaszek turbaszek Awaiting requested review from turbaszek

@mik-laj mik-laj Awaiting requested review from mik-laj

@kaxil kaxil Awaiting requested review from kaxil

@ashb ashb Awaiting requested review from ashb ashb is a code owner

Assignees
No one assigned
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow area:dev-tools area:providers provider:amazon-aws AWS/Amazon - related issues provider:google Google (including GCP) related issues type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@potiuk @ashb @eladkal @josh-fell @jedcunningham @ephraimbuddy