-
Notifications
You must be signed in to change notification settings - Fork 16.6k
Add task group ID filtering support to task instance query#58092
Add task group ID filtering support to task instance query#58092pierrejeambrun merged 3 commits intoapache:mainfrom
Conversation
closes: #55319
This PR is to address the 2nd issue of the above ticket.
Problem summary
When querying with task group name, the task instance list returns no results.
Proposed Solution
Added QueryTITaskDisplayNameOrGroupPattern filter in parameters.py that now supports
- The original search pattern
- An expansion for handling task group, when the search value matches a
task group, we now query all task IDs within the group.
After Fix
^ 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 airflow-core/newsfragments.
guan404ming
left a comment
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.
Overall looks good, one nit.
jason810496
left a comment
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.
Thanks for the PR!
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.
Nice! Thank you for the pull request.
I second what has been said above.
I'm not sure if we should or shouldn't have the same filter to handle both 'task pattern search' and 'group exact search'.
If we are to mix the two, it has to at least be consistent (exact search for all, or pattern search for all).
Maybe a separate distinct filter for QueryTaskGroupFilter would avoid that confusion and be simpler. (also this would create confusion if we have a task called 'something_xxxx' and a task group also called 'something'. Giving 'something' as a task group filter to the param would both get the something_xxxx task and all tasks from the something task group, which is basically wrong from a UI perspective -> task something_xxxx would appear in the list of tasks of the something group, if I'm not mistaken)
Also I think indeed that similarly to the TI we should probably filter on group_display_name and not group_id.
The advantage of this implementation is that it doesn't require front-end change to fix the issue though.
|
@yimingpeng That would be useful for other PRs/features too such as #58102. Any chance we can address the comment so we can move this forward? |
|
Hi @pierrejeambrun, apologies for the delay, just a bit swamped with some other things. I'll get this sorted within the week and thank you |
0013d17 to
d5dc5f6
Compare
|
Hello @guan404ming @jason810496 @pierrejeambrun , Thanks for your review comments and apologise for the delay. I think that I addressed all your feedbacks, pls help have another review when you have a chance, many thanks. Screen.Recording.2025-12-05.at.9.11.10.PM.mov |
jason810496
left a comment
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.
Thanks for the update!
pierrejeambrun
left a comment
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.
Thanks for the update.
I think we still need a few adjustments, and we should be good!
017c0e4 to
c0ed676
Compare
|
Thanks for insightful feedback, @jason810496 @pierrejeambrun Based on what has been suggested, I've made another update, please have another review when you have a chance, thanks . |
jason810496
left a comment
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.
Nice! Thanks for the quick update.
c0ed676 to
b5837ee
Compare
|
Hi @jason810496, thanks for the quick review and the good suggestions, just updated accordingly. |
jason810496
left a comment
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.
Only the final nit for import then the PR is good to go.
Thanks!
b5837ee to
61ff6bb
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.
Nice one LGTM.
I just pushed a nit. (renaming the filter to task_group_id to be consistent with the rest of the API)
Backport failed to create: v3-1-test. View the failure log Run details
You can attempt to backport this manually by running: cherry_picker 8256ee1 v3-1-test
This should apply the commit to the v3-1-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continue
|
* Add task group ID filtering support to task instance query
* Address the review comments
* Small Adjustment
---------
Co-authored-by: pierrejeambrun
(cherry picked from commit 8256ee1)
|
Manual backport #59511 |