-
Notifications
You must be signed in to change notification settings - Fork 14k
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
Allow per-timetable ordering override in grid view #25633
Conversation
What schedule does the DAG use? |
That one was a dataset-triggered. A regular schedule seems to work fine though |
Ah makes sense, I’ll add a special case for them. |
65f4b69
to
9bbd14d
Compare
@@ -3622,8 +3622,10 @@ def grid_data(self): | |||
if run_state: | |||
query = query.filter(DagRun.state == run_state) | |||
|
|||
dag_runs = query.order_by(DagRun.execution_date.desc()).limit(num_runs).all() | |||
ordering = (DagRun.__table__.columns[name].desc() for name in dag.timetable.run_ordering) |
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.
Should we have the UI read ordering
in order to know when to use data_interval_end
as a label instead of data_interval_start
?
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.
Yes, we should. I added a field dag_run_ordering
in the response JSON that’s a list of DAG run fields used for ordering. Would this be 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.
Yeah, that'll be good. But let's do that as a subsequent PR since it breaks a lot of tests and needs a bunch of UI changes.
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.
Reverted the addition.
39f7dbe
to
273c6b2
Compare
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.
Lgtm once tests pass and then I'll work on the UI part.
This comment was marked as outdated.
This comment was marked as outdated.
Previously dataset-triggered runs don't set the data interval an implicitly uses the compatibility fallback mechanism. This works fine, but creates weird result when the DAG runs are ordered by data interval fields because those NULL data intervals would sort differently against non-NULL values created by manual runs. This is fixed by explicitly providing the data interval when we create the dataset-triggered run, so those fields are populated when the run is created in the database.
These don't really have a notion of data intervals, so it's better to order them by logical date. The result shouldn't be different, but this makes the code easier to understand.
5328d5c
to
ed5228f
Compare
Close #25090.
I wonder if there are any other views that would benefit from this custom ordering logic.