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

SQLA20: Replace Session.query(...).get by Session.get #29205

Merged
merged 2 commits into from
Jan 28, 2023

Conversation

Taragolis
Copy link
Contributor

related: #28723

Replace abstract session.query(User).get(5) by session.get(User, 5) (1.4/2.0 compatible). First from the list: 2.0 Migration - ORM Usage

Note: most of the legacy usage found by regex: query\(.*\.get\( so there is possible that unchanged part still exists.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:Scheduler including HA (high availability) scheduler area:serialization area:webserver Webserver related Issues labels Jan 27, 2023
@Taragolis Taragolis changed the title SQLA20: Replace Query(pk).get by Session.get SQLA20: Replace Session.query(...).get by Session.get Jan 27, 2023
Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

I'm surprised there are so few!

@@ -3687,7 +3687,6 @@ def complete_one_dagrun():
for _ in range(5):
self.scheduler_job._do_scheduling(session)
complete_one_dagrun()
model: DagModel = session.query(DagModel).get(dag.dag_id)
Copy link
Member

Choose a reason for hiding this comment

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

Didn't need this one?

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 guess not, this model not use later in the code, we just make a query to DB and that all.

This part unchanged since it added in the end if 2021: #19528

@ashb
Copy link
Member

ashb commented Jan 28, 2023

Yeah, mostly because we have so many composite PKs, and using get() you have to match the column order directly, which is error prone/hard to read.

(Plus for whatever reason historically Airflow never use it. I think either due to a dislike of it, a misunderstanding of the SQLA session model or simply not knowing it exists)

@Taragolis Taragolis added the full tests needed We need to run full set of tests for this PR to merge label Jan 28, 2023
@Taragolis
Copy link
Contributor Author

Ok, lets do a full test. Just in case.

@kaxil kaxil merged commit 264ace5 into apache:main Jan 28, 2023
@pierrejeambrun pierrejeambrun added this to the Airflow 2.5.2 milestone Feb 27, 2023
@pierrejeambrun pierrejeambrun added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Feb 27, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 7, 2023
@Taragolis Taragolis deleted the alchemy20-query-get branch March 11, 2023 00:31
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:Scheduler including HA (high availability) scheduler area:serialization area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants