john-bodley commented on PR #22413: URL: https://github.com/apache/superset/pull/22413#issuecomment-1360219857
Thanks @dpgaspar for the detailed analysis. BTW I'm not sure that the two queries (which include the FAB change) are complete as they don't include the additional SELECT statements for fetching the columns and metrics and thus I'm also not sure if your timings include those facets. 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 https://github.com/apache/superset/pull/22332, which added the `/api/v1/dataset/<id>/column/` endpoint, per https://github.com/apache/superset/pull/22332#issuecomment-1344949577, I realized that this wasn't suffice given what is required by the frontent. -- 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
