Nataneljpwd commented on PR #53492: URL: https://github.com/apache/airflow/pull/53492#issuecomment-3162669328
> You seem to pretty quickly dismiss the notion that the query is .... pretty gnarly. You don't think it can be simplified? Or maybe you just want to get some data on performance first. Yes, we know the query is a monsterous one, as of now we want to get performance data and we also want to get it ready to be as an experimental feature in airflow 3.1 preferrably, we are working on thinking how to optimize the query, temp tables and materialized views may help with the performance, as of now from a very small test suite, the query takes between around half a second and a few seconds on different datasets, more data will be provided when further testing is done. > It's also, you know, I'm not sure that piling so much into one query is really required to solve starvation problems. But it is not a positive for intelligibility, and can make it harder to optimize. The nesting in particular I think is pretty tough for intelligibility. Maybe it's necessary but I'd be surprised. Me and @Asquator have talked about it in #45636 and we do need the 4 windows to solve all possible starvations (other than executor slots), if you find any flaw in there we would be very happy to hear your suggetsions. > > I did notice you are missing a join condition here: > > ``` > JOIN dag_run ON task_instance.run_id = dag_run.run_id > ``` > > This would result in join explosion if you have same run id for diff dags (and since airlfow's run id is derived from a date, it happens commonly). Hmm, don't we have this join? If we didnt have it, wouldnt it cause a Cartesian join? @Asquator worked on removing all Cartesian joins, maybe the query posted above has some parts missing of the actual query. > You may be able to avoid some joins to dag run if you are just after TI. Some of our relationships are eager loading and result in joins we don't need (but it can be disabled with query options). > > After taking a little closer look at the query.... > > I was struck by how often we go back and do a row number over the task instance table. Is that really necessary? I would have expected that maybe once we grab the pool of candidate TIs in the innermost select, that we'd be able to just successively filter that down to the final set, even if we do have to involve some nested window functions. Over and over it's select *, row_number() from TI, DR, inner_query. Is there no way to stop going back to TI and DR after the first result set? We will check if we can de-nest the windows, each window has a different order by (with some exceptions) and maybe reduce computations, but we do need to do some aggregation for each window in order for the query to compile, and we do need the window for the group by and partition by, so row number is used. > It also can be a nice thing to do (for readability and debugging) to provide an alias or label for your subselects and ctes (e.g. to avoid anon_6 etc. That will be done, we just want to get some testing done and then make it beautiful. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
