geido commented on PR #43719: URL: https://github.com/apache/superset/pull/43719#issuecomment-5512569049
Follow-up on the payload/perf question with measurements instead of reasoning, @amaannawab923 — including a correction to something I asserted in [that thread](https://github.com/apache/superset/pull/43719#discussion_r3915017830). **Database cost: one extra `SELECT` per request.** Counted statements during the HTTP request on `origin/master` vs this branch, 10-column / 10-metric dataset: | | master | branch | |---|---|---| | `SELECT`s during request | 12 | 13 | The extra one is the eager load of `sql_metrics`. Everything else is byte-identical — auth lookups, `table_columns`, subjects, `tables`. Returning every column rather than only dimensions costs nothing: it was always the same `table_columns` query, previously just filtered in Python afterwards. And the metrics query isn't avoidable-but-chosen — dropping `metrics.*` from `select_columns` would make the schema lazy-load the same rows at dump time. Eager is the cheaper of the two shapes. **Correction: no new requests for any default role.** I wrote that this PR "increases who fetches `drill_info`". That's wrong for every stock role, and I should have checked rather than reasoned. `ChartContextMenu` already fetched this endpoint whenever `canDrillToDetail || canDrillBy`, and both call sites build the same endpoint string, which `cachedSupersetGet` dedupes for the page's lifetime. Querying a `superset init`-seeded metadata DB for what Gamma actually holds: ``` Gamma HAS can_explore, can_get_drill_info, can_drill, can_write (ExploreFormDataRestApi), can_view_chart_as_table Gamma lacks can_samples on Datasource ``` `canDrillToDetail` is false — no `can_samples` — which is exactly why Gamma saw raw names. But `canDrillBy` is **true**, so Gamma's browser was already making this request. The fix just lets `SliceHeaderControls` read a response that was already cached. Net new network requests for Gamma, Alpha and Admin: zero. New fetches require a custom role with `can_get_drill_info` plus explore/view-table but lacking *both* `can_samples` and `can_write` on `ExploreFormDataRestApi`. **One cost that is real,** from @EnxDev's extension finding after that thread was resolved: where `load.drillby.options` is registered, the hook now also calls the REST endpoint for labels — one extra request per dataset per dashboard load, in those deployments only. Reasoning in [that thread](https://github.com/apache/superset/pull/43719#discussion_r3916107899). **Permissions: no boundary moves.** Route gate unchanged (`@protect()` → `can_get_drill_info` on Dataset), object gate unchanged (`DatasourceFilter`, and `can_drill_dataset_via_dashboard_access` on the dashboard fallback). Nobody can call the endpoint who couldn't before; users who now trigger it already held the permission and simply weren't exercising it. What changed is response content for already-entitled callers — the widening flagged above. One rough edge: a user holding `can_get_drill_info` but lacking access to that specific dataset now triggers a failing request where they previously triggered none. It degrades cleanly to raw names, but it is a console error that explore-only users didn't produce before. All of the above is now summarised in the PR description so it isn't buried in resolved threads. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
