amaannawab923 commented on code in PR #43719:
URL: https://github.com/apache/superset/pull/43719#discussion_r3906728179
##########
superset/datasets/api.py:
##########
@@ -1757,6 +1757,8 @@ def get_drill_info(self, pk: int, **kwargs: Any) ->
Response:
"columns.column_name",
"columns.verbose_name",
"columns.groupby",
+ "metrics.metric_name",
Review Comment:
`drill_info` is fetched per dataset on dashboard load, while the results
grid this fixes is opened on demand. On datasets with a large metric list,
every dashboard load now carries metrics that most sessions never read.
Likely negligible in practice, and the columns list being narrowed in
`post_dump` suggests payload size was a consideration here previously, so it
seems worth an explicit answer rather than an implicit one.
##########
superset/datasets/schemas.py:
##########
@@ -516,5 +525,9 @@ def post_dump(
]
if security_manager.is_guest_user():
- return {"id": serialized["id"], "columns": serialized["columns"]}
+ return {
+ "id": serialized["id"],
+ "columns": serialized["columns"],
+ "metrics": serialized.get("metrics", []),
Review Comment:
The guest branch previously returned a deliberately minimal payload, and the
columns in it are narrowed to `groupby=True` just above. Adding `metrics`
unfiltered means an embedded guest receives every metric defined on the
dataset, not only those used by the chart they are viewing. A `verbose_name` is
analyst-authored and can carry business context, so it is a wider surface than
the metric names alone suggest.
Worth noting that #43390 hardened this same schema on the grounds that
drill_info is guest-reachable through the dashboard fallback, and that change
is not on this branch yet. A rebase would put that reasoning next to the new
comment above.
If the results grid only needs labels for metrics already present in the
query result, scoping the guest branch to those would keep the payload matched
to what the caller can already see, and would keep both lists in this response
scoped consistently.
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -3399,6 +3410,13 @@ def test_get_drill_info_admin_user(self):
{"column_name": "category", "verbose_name": "Category Column"},
{"column_name": "region", "verbose_name": None},
]
+ # Metrics must also carry their verbose_name so that consumers (e.g.
+ # the dashboard "View as table" results grid) can resolve a metric's
+ # friendly Label instead of falling back to its technical name.
+ assert result["metrics"] == [
Review Comment:
Both new assertions cover metrics that should be returned. Nothing asserts
the deliberate part, which is that metrics are not filtered the way columns
are. Since that asymmetry is intentional and currently explained only in a
comment, a later change that makes the two consistent would still pass.
A fixture metric that would be excluded under column-style filtering,
asserted as present, would pin the intent in the suite rather than in prose.
--
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]
