-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Correctly select DagRun.execution_date from db #18421
Correctly select DagRun.execution_date from db #18421
Conversation
2fad057
to
fe45c39
Compare
.limit(1) | ||
.scalar() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.limit(1) | |
.scalar() | |
.scalar() |
I think using only .scalar()
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs say scalar()
fails if there’s more than one row
https://docs.sqlalchemy.org/en/14/orm/query.html#sqlalchemy.orm.Query.scalar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why you went with limit
instead of first
?
https://docs.sqlalchemy.org/en/14/orm/query.html#sqlalchemy.orm.Query.first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think first()
would require unpacking the returning tuple manually? This feels slightly easier to read for me than
min_date_row = session.query(...).first()
if min_date_row is None:
...
min_date = min_date_row[0]
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
555be7e
to
754d78c
Compare
b2c82b5
to
abda7d6
Compare
This comment has been minimized.
This comment has been minimized.
abda7d6
to
4c4c98c
Compare
The importance of test coverage and type annotations…