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 1) #31569

Merged
merged 10 commits into from
May 30, 2023

Conversation

ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented May 26, 2023

This commit updates the sqlalchemy queries to adopt the 2.0 query style,
which is compatible with version 1.4. The changes involve updating the engine
with the future=True flag to indicate the execution of queries using the 2.0 style.
As a result, textual SQL statements are wrapped with the text function.

In addition, queries that previously used delete and update operations have been
modified to utilize the new delete/update construct. Furthermore, all queries within the
jobs/ and api/ directories have been thoroughly updated to employ the new style queries.

Please note that this commit intentionally stops at this point to ensure ease of review for the pull request.
The only test change is the addition of the future flag to the create_engine function.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:CLI area:Scheduler including HA (high availability) scheduler area:webserver Webserver related Issues kind:documentation labels May 26, 2023
@ephraimbuddy ephraimbuddy force-pushed the sqlalchemy-20-query-style branch 8 times, most recently from ec72e1e to 0994ff2 Compare May 28, 2023 11:55
@ephraimbuddy ephraimbuddy changed the title Update sqlalchemy queries to 2.0 style Refactor sqlalchemy queries to 2.0 style (Part 1) May 28, 2023
@ephraimbuddy ephraimbuddy marked this pull request as ready for review May 28, 2023 11:56
@ephraimbuddy ephraimbuddy force-pushed the sqlalchemy-20-query-style branch 2 times, most recently from 872d853 to 334ebeb Compare May 28, 2023 13:55
Comment on lines 466 to 475
tis = session.execute(
select(TaskInstance).where(
TaskInstance.dag_id == dag.dag_id,
TaskInstance.run_id == run_id,
TaskInstance.task_id.in_(task_ids),
TaskInstance.state.in_([State.RUNNING, State.DEFERRED, State.UP_FOR_RESCHEDULE]),
)
)
task_ids_of_running_tis = [task_instance.task_id for task_instance in tis]

task_ids_of_running_tis = [task_instance.task_id for task_instance, in tis]
Copy link
Member

@uranusjr uranusjr May 29, 2023

Choose a reason for hiding this comment

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

Note: This can be rewritten with scalars on TaskInstance.task_id instead. (Maybe in a later PR.)

airflow/jobs/job.py Outdated Show resolved Hide resolved
@ephraimbuddy ephraimbuddy force-pushed the sqlalchemy-20-query-style branch from 46e0043 to 6708ec9 Compare May 29, 2023 09:04
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it’s weird these screenshots are being updated. The added colouring also is suspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran breeze setup regenerate-command-images --force when I couldn't find why the PR was failing on pre-commit generating ER diagram(not locally)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, what I was trying to say is I don’t understand why pre-commit thinks they need to be updated at all.

@ephraimbuddy ephraimbuddy force-pushed the sqlalchemy-20-query-style branch from f369550 to 6722b44 Compare May 29, 2023 13:21
airflow/jobs/job.py Outdated Show resolved Hide resolved
Comment on lines +361 to +363
dag_run = session.execute(
select(DagRun).where(DagRun.dag_id == dag_id, DagRun.run_id == run_id)
).scalar_one()
Copy link
Member

Choose a reason for hiding this comment

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

Should be equivalent to

Suggested change
dag_run = session.execute(
select(DagRun).where(DagRun.dag_id == dag_id, DagRun.run_id == run_id)
).scalar_one()
dag_run = session.scalars(
select(DagRun).where(DagRun.dag_id == dag_id, DagRun.run_id == run_id)
).one()

but I don’t think this is better. So just for reference in case anyone wonders.

ephraimbuddy and others added 10 commits May 30, 2023 07:23
This commit updates the sqlalchemy queries to adopt the 2.0 query style,
which is compatible with version 1.4. The changes involve updating the engine
with the future=True flag to indicate the execution of queries using the 2.0 style.
As a result, textual SQL statements are wrapped with the text function.

In addition, queries that previously used delete and update operations have been
modified to utilize the new delete/update construct. Furthermore, all queries within the
jobs/ and api/ directories have been thoroughly updated to employ the new style queries.

Please note that this commit intentionally stops at this point to ensure ease of review for the pull request.
The only test change is the addition of the future flag to the create_engine function.
@ephraimbuddy ephraimbuddy force-pushed the sqlalchemy-20-query-style branch from 6722b44 to c64be9f Compare May 30, 2023 06:23
@ephraimbuddy ephraimbuddy merged commit 0f1cef2 into apache:main May 30, 2023
@ephraimbuddy ephraimbuddy deleted the sqlalchemy-20-query-style branch May 30, 2023 16:52
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Jul 6, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:CLI area:Scheduler including HA (high availability) scheduler area:webserver Webserver related Issues kind:documentation type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants