dpgaspar commented on PR #22413: URL: https://github.com/apache/superset/pull/22413#issuecomment-1369831570
> Thanks @dpgaspar for the detailed analysis. BTW I'm not sure that the two queries which include the FAB change (and their associated timings) are complete as they don't include the additional SELECT statements for fetching the columns and metrics. > > The TL;DR—regarding performance—from you analysis is: > > FAB Superset (FWD) Superset (BWD) Endpoint Response Time Difference > `joined` `joined`* `select` /api/v1/dashboard/ ~ 27 ms N/A > `joined` `joined` * `select` /api/v1/dashboard/1 ~ 2,300 ms N/A > `default` `select` `select` /api/v1/dashboard/ ~ 61 ms + 34 ms (+ 126%) > `default` `select` `select` /api/v1/dashboard/1 ~ 875 ms - 1,425 ms (- 62%) > `default` `selectin` `joined` /api/v1/dashboard/ ~ 61 ms + 34 ms (+ 126%) > `default` `selectin` `joined` /api/v1/dashboard/1 ~ 610 ms - 1,690 ms (- 73.5%) > * By enforcement via FAB. > > which indicates that there is a performance hit (albeit relatively small from an absolute perspective) for the `/api/v1/dashboard/` endpoint, however there is a signification performance improvement (from both an absolute and relative perspective) for the `/api/v1/dashboard/1` endpoint, i.e., the results seem quite favorable. > > Regarding the get list concern—generating N + 1 queries—I'm not sure if this will materialize given we're using the [selectin` eager loading](https://docs.sqlalchemy.org/en/14/orm/loading_relationships.html#selectin-eager-loading) which mentions that, > > > In most cases, selectin loading is the most simple and efficient way to eagerly load collections of objects. > > i.e., though multiple `SELECT` statements will exists, the number is determined via the number of relationships as opposed to the number of items in the list/collection. > > Additionally with regards to #22332, which added the `/api/v1/dataset/<id>/column/` endpoint, per [#22332 (comment)](https://github.com/apache/superset/pull/22332#issuecomment-1344949577), I realized that this wasn't suffice given what is required by the frontent. Ok, I've added a small change on FAB on top of your PR there, now we can have the best from both worlds: https://github.com/dpgaspar/Flask-AppBuilder/pull/1971 This is included on 4.2.0 So use: `show_outer_default_load = True` RestModelApi attr when needed to improve performance `/api/v1/dataset/<id>` -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
