Uh oh!
There was an error while loading. Please reload this page.
AIP-84 Add multisort to dags list request #46383 - #47440
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
pierrejeambrun
left a comment
There was a problem hiding this comment.
Nice, we need some tests too.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
pierrejeambrun
commented
Mar 7, 2025
Don't hesitate to |
prasad-madine
commented
Mar 7, 2025
Yes, currently working on that, will update the PR once it is done. |
prasad-madine
commented
Mar 7, 2025
I thought to do it after writing test cases. |
prasad-madine
commented
Mar 13, 2025
Hi @pierrejeambrun , I have added few tests, COuld you please review it and let me know if any additional things to be required. |
…bug-add-multisort-to-dags-list-request-apache#46383
pierrejeambrun
left a comment
There was a problem hiding this comment.
Thanks for the PR. Looking good overall. One couple of comments only.
CI need fixing (tests, static check etc.), can you also rebase the branch solve conflicts.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…bug-add-multisort-to-dags-list-request-apache#46383
prasad-madine
commented
Mar 27, 2025
@pierrejeambrun could you please review the PR, If any changes needs to be done, I'll address those. |
There was a problem hiding this comment.
Thanks.
A few comments. Also the CI needs to be fixed:
- You can install pre-commits to run static checks locally. https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst
- You can start test locally with a local virtualenv and pytest or with breeze. All of that is described in the contribution docs (https://github.com/apache/airflow/tree/main/contributing-docs, https://github.com/apache/airflow/blob/main/contributing-docs/07_local_virtualenv.rst
https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst)
| self.to_replace = to_replace | ||
| def to_orm(self, select: Select) -> Select: | ||
| MAX_SORT_PARAMS = 5 |
There was a problem hiding this comment.
I think the limit should be made greater.
We already have table with more than 5 columns and the UI could reach that pretty soon.
Going for 10 or 20 shouldn't hurt. (all our different db should be able to handle much more anyway)
| def inner(order_by: list[str] | str | None = Query(default)) -> SortParam: | ||
| if order_by is None: | ||
| order_by = [self.get_primary_key_string()] | ||
| elif isinstance(order_by, str): | ||
| order_by = [order_by] | ||
| return self.set_value(order_by) |
There was a problem hiding this comment.
I'm not sure it's exactly equivalent. If there is no order_by query param specified, we shouldn't add any by default and let the query default order operate:
- order_by = None => don't update the query at all and let the default ordering of the query operate.
- order_by = [] or [""] -> fill with [primary_key_string] (or raise validation error)
| ({"order_by": ["last_run_state", "dag_display_name"], "only_active": False}, 3, [DAG1_ID, DAG3_ID, DAG2_ID]), | ||
| ({"order_by": ["-last_run_state", "dag_display_name"], "only_active": False}, 3, [DAG3_ID, DAG1_ID, DAG2_ID]), | ||
| ({"order_by": ["last_run_start_date", "dag_display_name"], "only_active": False}, 3, [DAG1_ID, DAG3_ID, DAG2_ID]), | ||
| ({"order_by": ["-last_run_start_date", "dag_display_name"], "only_active": False}, 3, [DAG3_ID, DAG1_ID, DAG2_ID]), | ||
| ({"order_by": ["-dag_id", "dag_display_name"]}, 2, [DAG2_ID, DAG1_ID]), | ||
| ({"order_by": ["dag_id", "dag_display_name"]}, 2, [DAG1_ID, DAG2_ID]), | ||
| ({"order_by": ["dag_display_name", "dag_id"]}, 2, [DAG1_ID, DAG2_ID]), | ||
| ({"order_by": ["last_run_state"], "only_active": False}, 3, [DAG1_ID, DAG3_ID, DAG2_ID]), | ||
| ({"order_by": ["-last_run_state"], "only_active": False}, 3, [DAG3_ID, DAG1_ID, DAG2_ID]), | ||
| ({"order_by": ["last_run_start_date"], "only_active": False}, 3, [DAG1_ID, DAG3_ID, DAG2_ID]), | ||
| ({"order_by": ["-last_run_start_date"], "only_active": False}, 3, [DAG3_ID, DAG1_ID, DAG2_ID]) | ||
| ({"order_by": ["dag_display_name", "-next_dagrun"]}, 2, [DAG1_ID, DAG2_ID]), | ||
| ({"order_by": ["last_run_state", "-dag_display_name", "dag_id"]}, 2, [DAG1_ID, DAG2_ID]), | ||
| ({"order_by": ["-last_run_start_date", "dag_display_name", "next_dagrun", "dag_id"]}, 2, [ DAG1_ID, DAG2_ID]), | ||
| ({"order_by": ["dag_display_name", "-last_run_state", "next_dagrun", "dag_id", "last_run_start_date"]}, 2, [DAG1_ID, DAG2_ID]), |
There was a problem hiding this comment.
We still need two tests with:
({"order_by": ["criteria_1", "criteria_2"]}, 2, [id1, id2, id3]),
({"order_by": ["criteria_1", "-criteria_2"]}, 2, [different sequence of id, proving that criteria_2 is being considerd]),
| elif isinstance(order_by, str): | ||
| order_by = [order_by] |
There was a problem hiding this comment.
This shouldn't be possible. FastAPI validation will prevent such wrong input and will do what's necessary to either get a list[str] or fail with a 422 validation error if not possible.
| def inner(order_by: str = default or self.get_primary_key_string()) -> SortParam: | ||
| return self.set_value(self.get_primary_key_string() if order_by == "" else order_by) | ||
| def dynamic_depends(self, default: list[str] | None = None) -> Callable: | ||
| def inner(order_by: list[str] | str | None = Query(default)) -> SortParam: |
There was a problem hiding this comment.
| definner(order_by: list[str] |str|None=Query(default)) ->SortParam: | |
| definner(order_by: list[str] |None=Query(default)) ->SortParam: |
pierrejeambrun
commented
Apr 9, 2025
@prasad-madine are you able to move forward, or do you need help ? |
@pierrejeambrun I've made changes, but facing issues in setup with latest code, I need to test it. I've also tried to setup application again. Need some help. |
pierrejeambrun
commented
Apr 14, 2025
Make sure your images have been rebuilt to be up to date. (When you start breeze you should see a prompt asking if you want to rebuild or not, you need to answer You can try in Otherwise you can stop breeze then run If the issue persists you can try |
prasad-madine
commented
Apr 14, 2025
I've also tried incognito and also tried removing networks, volumes and docker images and rebuilt the application again, but still facing the same issue. |
pierrejeambrun
commented
Apr 14, 2025
Something must be wrong, you can try on main. Be sure to rebuild images. It looks like the connection is refused, the api-server is not reached. Also you can try with |
pierrejeambrun
commented
Apr 14, 2025
That might be wrong on the auth manager asset compilation side for |
pierrejeambrun
commented
Apr 16, 2025
@prasad-madine did you manage to solve your local setup issue ? |
pierrejeambrun
commented
May 15, 2025
@prasad-madine Any progress on that? |
prasad-madine
commented
May 15, 2025
No @pierrejeambrun , still the issue with application setup is not resolved. |
This does not happen on latest main. You can rebuild the images and start breeze again, you should be able to access the UI. Breeze documentation can be found here https://github.com/apache/airflow/tree/main/dev/breeze/doc. You can run a |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
pierrejeambrun
commented
Jul 9, 2025
I am working on this, and will open a separate PR when it's ready |
pierrejeambrun
commented
Jul 16, 2025
Closing in favor of #53408 |


Description:
backend part of: #46383
Test cases screenshot