mikebridge commented on code in PR #43838:
URL: https://github.com/apache/superset/pull/43838#discussion_r3992972103
##########
superset/versioning/activity/impact.py:
##########
@@ -159,28 +179,56 @@ def batch_chart_counts(
row["slice_end"] is None or row["slice_end"] > target_tx
)
if in_m2m and in_slice:
- matches.setdefault((ds_id, target_tx),
set()).add(row["slice_id"])
+ matches.setdefault((ds_id, target_tx), {})[row["slice_id"]] = (
Review Comment:
Closed by the rebase onto master's attach/detach membership (sc-119907):
`batch_chart_impacts` now resolves the names through
`charts_attached_to_dashboard`'s `[attach, detach)` windows, so a chart
detached before the transaction is no longer listed — the m2m
`end_transaction_id` is never consulted. Thanks for catching it before it
shipped as a named list.
##########
superset/versioning/activity/impact.py:
##########
@@ -50,6 +52,19 @@
)
+class ChartRef(TypedDict):
+ """One affected chart in an ``impact`` payload: id plus
name-at-transaction."""
+
+ id: int
+ name: str
+
+
+# The wire ``chart_names`` list is capped so one dataset feeding very many
+# charts cannot balloon every related record on the page (page sizes reach
+# 200 records); ``charts`` always carries the full count.
+IMPACT_CHART_NAMES_CAP = 50
Review Comment:
Reworded in 129f6eb35f to say exactly what it bounds — one record's list (a
per-tooltip number), applied per record in `impact_for_record`, so a 200-record
page can carry 200 × the cap. Value unchanged, per your note.
##########
superset/versioning/schemas.py:
##########
@@ -267,6 +281,17 @@ class ActivityImpactSchema(Schema):
)
},
)
+ chart_names = fields.List(
Review Comment:
Renamed to `affected_charts` in 129f6eb35f (backend, schema, OpenAPI,
frontend consumer). The cap constant now lives with the contract in
`schemas.py` and the field description interpolates it, so there is no literal
50 to go stale.
##########
tests/unit_tests/versioning/test_activity.py:
##########
@@ -480,46 +481,130 @@ def test_changed_by_projects_only_display_fields() ->
None:
# ---- impact_for_record (pure, post-batch) -------------------------------
-def test_impact_for_record_dashboard_path_dataset_related_uses_count() -> None:
+def test_impact_for_record_dashboard_path_dataset_related_uses_charts() ->
None:
"""The only path/related shape that carries impact: ``Dashboard`` →
- ``SqlaTable``. The count comes from the pre-batched lookup."""
+ ``SqlaTable``. Count and names both come from the pre-batched lookup
+ (sc-119775: the tooltip needs the names, not just the count)."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- counts = {(5, 100): 3}
- assert impact_for_record(record, "Dashboard", counts) == {"charts": 3}
+ charts: list[ChartRef] = [
+ {"id": 11, "name": "Alpha"},
+ {"id": 12, "name": "Beta"},
+ {"id": 13, "name": "Gamma"},
+ ]
+ assert impact_for_record(record, "Dashboard", {(5, 100): charts}) == {
+ "charts": 3,
+ "chart_names": charts,
+ }
-def test_impact_for_record_missing_count_yields_none() -> None:
+def test_impact_for_record_missing_pair_yields_none() -> None:
"""A pair the batch query didn't return (no matching siblings)
collapses to ``None`` rather than ``{"charts": 0}``."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
assert impact_for_record(record, "Dashboard", {}) is None
-def test_impact_for_record_zero_count_yields_none() -> None:
- """Explicit zero in the counts map is treated the same as missing —
- no impact field on the wire."""
+def test_impact_for_record_empty_charts_yields_none() -> None:
+ """An explicit empty list in the impacts map is treated the same as
+ missing — no impact field on the wire."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "Dashboard", {(5, 100): 0}) is None
+ assert impact_for_record(record, "Dashboard", {(5, 100): []}) is None
def test_impact_for_record_dashboard_path_chart_related_yields_none() -> None:
"""Dashboard → chart is a direct dependency; no further sibling
layer to count."""
record = {"entity_kind": "chart", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "Dashboard", {(5, 100): 999}) is None
+ charts: list[ChartRef] = [{"id": 9, "name": "X"}]
+ assert impact_for_record(record, "Dashboard", {(5, 100): charts}) is None
def test_impact_for_record_chart_path_with_dataset_related_yields_none() ->
None:
"""Chart → dataset: the chart is itself the only dependent of the
dataset edit."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "Slice", {(5, 100): 999}) is None
+ charts: list[ChartRef] = [{"id": 9, "name": "X"}]
+ assert impact_for_record(record, "Slice", {(5, 100): charts}) is None
def test_impact_for_record_dataset_path_yields_none() -> None:
"""Datasets have no transitive layer (AV-004)."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "SqlaTable", {(5, 100): 999}) is None
+ charts: list[ChartRef] = [{"id": 9, "name": "X"}]
+ assert impact_for_record(record, "SqlaTable", {(5, 100): charts}) is None
+
+
+def test_sorted_chart_refs_orders_case_insensitively_with_id_tiebreak() ->
None:
+ """The wire order is deterministic: casefolded name, then id; empty
+ names sort first (they render as an Untitled fallback)."""
+ from superset.versioning.activity.impact import _sorted_chart_refs
+
+ refs = _sorted_chart_refs({(5, 100): {3: "beta", 1: "Alpha", 2: "alpha",
4: ""}})
+ assert refs == {
+ (5, 100): [
+ {"id": 4, "name": ""},
+ {"id": 1, "name": "Alpha"},
+ {"id": 2, "name": "alpha"},
+ {"id": 3, "name": "beta"},
+ ]
+ }
Review Comment:
Applied verbatim in 129f6eb35f — thanks for measuring the three mutations.
--
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]
