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

Refactor Sqlalchemy queries to 2.0 style (Part 3) #32177

Merged
merged 5 commits into from
Jul 3, 2023

Conversation

phanikumv
Copy link
Contributor

This is a continuation of the effort to refactor the queries to sqlalchemy 2.0

cc @ephraimbuddy @uranusjr

related: #28723


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

@phanikumv phanikumv requested a review from ephraimbuddy June 27, 2023 09:28
@phanikumv phanikumv force-pushed the sqlalchemy_changes branch from 09a3e7d to 66e71dc Compare June 27, 2023 09:29
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Changes look good to me

airflow/utils/db.py Show resolved Hide resolved
airflow/utils/db.py Show resolved Hide resolved
airflow/utils/db.py Show resolved Hide resolved
@phanikumv phanikumv force-pushed the sqlalchemy_changes branch 4 times, most recently from 0d68ef2 to 52bb22e Compare June 27, 2023 16:19
@@ -1304,10 +1306,10 @@ def _dangling_against_task_instance(session, source_table, dag_run, task_instanc
)

return (
session.query(*[c.label(c.name) for c in source_table.c])
select(*[c.label(c.name) for c in source_table.c])
Copy link
Member

Choose a reason for hiding this comment

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

Let’s also change this to select(*(...)) (in the other planned PR). There are several of these around in this file too.

Copy link
Member

Choose a reason for hiding this comment

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

@uranusjr Creating a tuple rather than a list is more efficient?

Copy link
Member

Choose a reason for hiding this comment

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

This does not create an intermediate tuple but expands a generator directly. With *[] the generator is first realised into a list and then the list is expanded.

@phanikumv phanikumv force-pushed the sqlalchemy_changes branch from 52bb22e to 0660b4e Compare June 28, 2023 12:27
@phanikumv phanikumv added the use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) label Jun 28, 2023
@phanikumv phanikumv closed this Jun 28, 2023
@phanikumv phanikumv reopened this Jun 28, 2023
@phanikumv phanikumv force-pushed the sqlalchemy_changes branch 2 times, most recently from 71a2ee7 to 54d3593 Compare July 2, 2023 12:51
@phanikumv phanikumv marked this pull request as ready for review July 2, 2023 13:41
@phanikumv phanikumv requested a review from uranusjr July 3, 2023 05:10
airflow/utils/db.py Outdated Show resolved Hide resolved
airflow/utils/db.py Outdated Show resolved Hide resolved
airflow/utils/db.py Outdated Show resolved Hide resolved
airflow/www/views.py Outdated Show resolved Hide resolved
airflow/www/views.py Outdated Show resolved Hide resolved
@phanikumv phanikumv force-pushed the sqlalchemy_changes branch from 54d3593 to c56bafc Compare July 3, 2023 10:25
@phanikumv phanikumv merged commit 1065687 into apache:main Jul 3, 2023
@phanikumv phanikumv deleted the sqlalchemy_changes branch July 3, 2023 12:19
potiuk added a commit to potiuk/airflow that referenced this pull request Jul 4, 2023
potiuk added a commit that referenced this pull request Jul 4, 2023
@potiuk
Copy link
Member

potiuk commented Jul 4, 2023

As discussed in #32343 - this one is reverted and needs to be reapplied with "full tests needed" :(.

@phanikumv phanikumv added the full tests needed We need to run full set of tests for this PR to merge label Jul 4, 2023
@phanikumv phanikumv restored the sqlalchemy_changes branch July 4, 2023 16:25
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone Jul 6, 2023
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge type:misc/internal Changelog: Misc changes that should appear in change log use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants