You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by kg...@apache.org on 2026/05/29 16:26:19 UTC
(superset) 12/23: perf(activity-view): batch impact-count query to remove N+1
This is an automated email from the ASF dual-hosted git repository.
kgabryje pushed a commit to branch sc-107283-versioning-activity-view
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 4ead3f405f7c7b9ed64ade18cca57dc77e53e5ef
Author: Mike Bridge <mi...@ext.preset.io>
AuthorDate: Tue May 26 16:10:40 2026 -0600
perf(activity-view): batch impact-count query to remove N+1
Surfaced by the SQLAlchemy review (Warning #1) and required before
T045's p95 perf budget can be met on dashboards with many historical
dataset edits.
Before: _decorate_records fired one COUNT(DISTINCT slice_id) query
per related dataset record via _compute_impact → _count_dashboard_
charts_pointing_at_dataset_at_tx. For a dashboard activity stream
with N dataset-edit records, that's N round-trips to compute impacts.
After: _decorate_records collects the distinct (dataset_id, target_tx)
pairs once via _collect_impact_pairs, fires a single batched query
via _batch_chart_counts that pulls the (slice, dataset, validity-
window) state for every relevant slice, and filters by validity in
Python per pair. The result is a {(dataset_id, target_tx): count}
mapping consumed by the new pure helper _impact_for_record per row.
Round-trip count drops from O(N records) to O(1) for the impact
calculation. The SQL stays small and dialect-portable — same per-kind
IN-clause + Python validity filter pattern as _fetch_change_records.
Trade-off documented in _batch_chart_counts' docstring: the SELECT
pulls (slice_id, datasource_id, two validity-window pairs) for every
slice ever on the dashboard whose dataset matches one of the
requested dataset_ids. For a busy dashboard with 100 slices and 50
versions each, that's ~5000 rows into Python vs N small COUNT scans.
For N > ~5 (which is typical) the batch wins.
Removed:
* _compute_impact (replaced by _impact_for_record + _collect_impact_pairs)
* _count_dashboard_charts_pointing_at_dataset_at_tx (replaced by
_batch_chart_counts)
Test changes: the three _compute_impact unit tests (no-impact paths)
become six _impact_for_record tests (positive count + four no-impact
paths + zero-count → None). Five new _collect_impact_pairs tests
cover dashboard/chart/dataset path branching plus dedupe and empty.
Full suite: 27/27 integration + 65/65 unit (was 57; +8 from the
restructure). No semantic regression on either side of the cut.
Co-Authored-By: Claude Opus 4.7 (1M context) <no...@anthropic.com>
---
superset/versioning/activity.py | 144 +++++++++++++++++----------
tests/unit_tests/versioning/test_activity.py | 114 +++++++++++++++------
2 files changed, 177 insertions(+), 81 deletions(-)
diff --git a/superset/versioning/activity.py b/superset/versioning/activity.py
index bcf083cbf15..76a5af49773 100644
--- a/superset/versioning/activity.py
+++ b/superset/versioning/activity.py
@@ -497,52 +497,49 @@ def _denormalize_entity_names(records: list[dict[str, Any]]) -> list[dict[str, A
return records
-# ---- T010: Sibling-count impact -------------------------------------------
+# ---- T010: Sibling-count impact (batched) ---------------------------------
-def _compute_impact(
- path_kind: str, path_id: int, related_record: dict[str, Any]
-) -> Optional[dict[str, int]]:
- """Synthesize the ``impact`` field for one ``source='related'`` record.
-
- Per data-model.md §"``impact`` computation":
-
- * ``path=Dashboard`` and the related entity is a ``Slice``: no impact
- (the chart is a direct child of the dashboard; there's no further
- dependent layer).
- * ``path=Dashboard`` and the related entity is a ``SqlaTable``:
- ``{"charts": N}`` where N is the count of distinct charts on the
- path dashboard that pointed at the dataset at the change's
- ``transaction_id``.
- * ``path=Slice`` and the related entity is a ``SqlaTable``: no impact
- (the chart is itself the only dependent of the dataset edit).
- * Other shapes: no impact.
+def _collect_impact_pairs(
+ records: list[dict[str, Any]], path_kind: str
+) -> set[tuple[int, int]]:
+ """Distinct ``(dataset_id, transaction_id)`` pairs from *records*
+ that require an impact computation per data-model.md.
+
+ Only dashboard-path records whose related entity is a ``SqlaTable``
+ produce a non-null ``impact`` field; for any other shape this set
+ is empty and no DB query needs to fire.
"""
- table_kind = related_record["entity_kind"]
- api_kind = _TABLE_KIND_TO_API.get(table_kind)
- if path_kind != "Dashboard" or api_kind != "SqlaTable":
- return None
+ if path_kind != "Dashboard":
+ return set()
+ return {
+ (record["entity_id"], record["transaction_id"])
+ for record in records
+ if _TABLE_KIND_TO_API.get(record["entity_kind"]) == "SqlaTable"
+ }
- dataset_id = related_record["entity_id"]
- target_tx = related_record["transaction_id"]
- chart_count = _count_dashboard_charts_pointing_at_dataset_at_tx(
- path_id, dataset_id, target_tx
- )
- if chart_count == 0:
- return None
- return {"charts": chart_count}
+def _batch_chart_counts(
+ dashboard_id: int, pairs: set[tuple[int, int]]
+) -> dict[tuple[int, int], int]:
+ """For every ``(dataset_id, target_tx)`` in *pairs*, count the
+ distinct charts that were both on *dashboard_id* and pointing at
+ *dataset_id* at *target_tx*.
-def _count_dashboard_charts_pointing_at_dataset_at_tx(
- dashboard_id: int, dataset_id: int, target_tx: int
-) -> int:
- """Count distinct slices that were both (a) on *dashboard_id* at
- *target_tx* and (b) pointing at *dataset_id* at *target_tx*.
+ One SELECT against ``dashboard_slices_version`` ⨝ ``slices_version``,
+ pulling the (slice, dataset, validity-window) state for every slice
+ ever on the dashboard whose dataset matches one of the requested
+ dataset_ids. The Python loop then applies the validity-strategy
+ predicate per pair. Replaces the previous N+1 shape that fired one
+ COUNT per related record.
- Joins ``dashboard_slices_version`` ⨝ ``slices_version`` with the
- validity-strategy predicate on each side (same shape as
- :func:`superset.versioning.changes._dashboard_slice_uuids_at_tx`).
+ Returns ``{(dataset_id, target_tx): count}``; pairs whose count
+ would be zero are omitted so the caller's ``.get(key, 0)`` is
+ correct.
"""
+ if not pairs:
+ return {}
+
# pylint: disable=import-outside-toplevel
from sqlalchemy_continuum import version_class
@@ -552,27 +549,63 @@ def _count_dashboard_charts_pointing_at_dataset_at_tx(
m2m_tbl = metadata.tables.get("dashboard_slices_version")
slices_tbl = version_class(Slice).__table__
if m2m_tbl is None:
- return 0
+ return {}
- stmt = sa.select(sa.func.count(sa.distinct(m2m_tbl.c.slice_id))).where(
+ dataset_ids = {dataset_id for dataset_id, _ in pairs}
+ stmt = sa.select(
+ m2m_tbl.c.slice_id,
+ slices_tbl.c.datasource_id,
+ m2m_tbl.c.transaction_id.label("m2m_start"),
+ m2m_tbl.c.end_transaction_id.label("m2m_end"),
+ slices_tbl.c.transaction_id.label("slice_start"),
+ slices_tbl.c.end_transaction_id.label("slice_end"),
+ ).where(
m2m_tbl.c.dashboard_id == dashboard_id,
- m2m_tbl.c.transaction_id <= target_tx,
- sa.or_(
- m2m_tbl.c.end_transaction_id.is_(None),
- m2m_tbl.c.end_transaction_id > target_tx,
- ),
m2m_tbl.c.operation_type != 2,
slices_tbl.c.id == m2m_tbl.c.slice_id,
- slices_tbl.c.datasource_id == dataset_id,
+ slices_tbl.c.datasource_id.in_(dataset_ids),
slices_tbl.c.datasource_type == "table",
- slices_tbl.c.transaction_id <= target_tx,
- sa.or_(
- slices_tbl.c.end_transaction_id.is_(None),
- slices_tbl.c.end_transaction_id > target_tx,
- ),
slices_tbl.c.operation_type != 2,
)
- return int(db.session.connection().execute(stmt).scalar() or 0)
+ rows = db.session.connection().execute(stmt).all()
+
+ # For each pair, collect the slice_ids whose two validity windows
+ # both straddle target_tx. ``set`` dedupes within a pair.
+ matches: dict[tuple[int, int], set[int]] = {}
+ pairs_by_dataset: dict[int, list[int]] = {}
+ for dataset_id, target_tx in pairs:
+ pairs_by_dataset.setdefault(dataset_id, []).append(target_tx)
+
+ for slice_id, ds_id, m_start, m_end, s_start, s_end in rows:
+ for target_tx in pairs_by_dataset.get(ds_id, ()):
+ in_m2m = m_start <= target_tx and (m_end is None or m_end > target_tx)
+ in_slice = s_start <= target_tx and (s_end is None or s_end > target_tx)
+ if in_m2m and in_slice:
+ matches.setdefault((ds_id, target_tx), set()).add(slice_id)
+
+ return {pair: len(slice_ids) for pair, slice_ids in matches.items()}
+
+
+def _impact_for_record(
+ record: dict[str, Any],
+ path_kind: str,
+ counts: dict[tuple[int, int], int],
+) -> Optional[dict[str, int]]:
+ """Synthesize the ``impact`` field for one record using the pre-
+ fetched *counts* mapping. Pure function — no DB.
+
+ Per data-model.md §"``impact`` computation": only
+ ``path=Dashboard`` and ``related=SqlaTable`` shapes carry an
+ impact; everything else returns ``None``.
+ """
+ api_kind = _TABLE_KIND_TO_API.get(record["entity_kind"])
+ if path_kind != "Dashboard" or api_kind != "SqlaTable":
+ return None
+ key = (record["entity_id"], record["transaction_id"])
+ chart_count = counts.get(key, 0)
+ if chart_count == 0:
+ return None
+ return {"charts": chart_count}
# ---- T014: Live-row existence + soft-delete state -------------------------
@@ -778,6 +811,11 @@ def _decorate_records(
}
tombstones = _check_entity_tombstones(distinct)
uuids = _lookup_entity_uuids(distinct, tombstones)
+ # Pre-compute impact counts for the whole page in one batch query
+ # instead of one COUNT per related record (was N+1).
+ impact_counts = _batch_chart_counts(
+ path_id, _collect_impact_pairs(records, path_kind)
+ )
for record in records:
api_kind = _TABLE_KIND_TO_API.get(record["entity_kind"], "")
@@ -805,7 +843,7 @@ def _decorate_records(
record["impact"] = None
else:
record["summary"] = _build_summary(api_kind, record)
- record["impact"] = _compute_impact(path_kind, path_id, record)
+ record["impact"] = _impact_for_record(record, path_kind, impact_counts)
# Strip the internal-only columns the API contract doesn't expose.
for key in (
diff --git a/tests/unit_tests/versioning/test_activity.py b/tests/unit_tests/versioning/test_activity.py
index f099fbc4df4..b0cd02c63c3 100644
--- a/tests/unit_tests/versioning/test_activity.py
+++ b/tests/unit_tests/versioning/test_activity.py
@@ -39,8 +39,9 @@ from superset.versioning.activity import (
_build_summary,
_can_read,
_changed_by_dict,
- _compute_impact,
+ _collect_impact_pairs,
_DEFAULT_PAGE_SIZE,
+ _impact_for_record,
_intersect_windows,
_MAX_PAGE_SIZE,
_merge_entity_windows,
@@ -311,39 +312,96 @@ def test_can_read_returns_true_for_unsupported_kind() -> None:
assert _can_read("UnknownKind", object(), _StubSecurityManager()) is True
-# ---- _compute_impact no-impact paths -------------------------------------
+# ---- _impact_for_record (pure, post-batch) -------------------------------
-def test_compute_impact_self_path_yields_no_impact() -> None:
- """The chart-self case: a dashboard's chart-related record has no
- further dependent layer (chart is a direct child of dashboard)."""
- record = {
- "entity_kind": "chart",
- "entity_id": 5,
- "transaction_id": 100,
- }
- assert _compute_impact("Dashboard", 1, record) is None
+def test_impact_for_record_dashboard_path_dataset_related_uses_count() -> None:
+ """The only path/related shape that carries impact: ``Dashboard`` →
+ ``SqlaTable``. The count comes from the pre-batched lookup."""
+ record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
+ counts = {(5, 100): 3}
+ assert _impact_for_record(record, "Dashboard", counts) == {"charts": 3}
-def test_compute_impact_chart_path_with_dataset_related_yields_no_impact() -> None:
- """Chart → dataset: the chart itself is the only dependent of the
- dataset edit, so no impact count."""
- record = {
- "entity_kind": "dataset",
- "entity_id": 5,
- "transaction_id": 100,
- }
- assert _compute_impact("Slice", 1, record) is None
+def test_impact_for_record_missing_count_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_compute_impact_unknown_path_kind_yields_no_impact() -> None:
- """Defensive fallthrough — any other shape returns no impact."""
- record = {
- "entity_kind": "dataset",
- "entity_id": 5,
- "transaction_id": 100,
- }
- assert _compute_impact("SqlaTable", 1, record) 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."""
+ record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
+ assert _impact_for_record(record, "Dashboard", {(5, 100): 0}) 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
+
+
+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
+
+
+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
+
+
+# ---- _collect_impact_pairs -----------------------------------------------
+
+
+def test_collect_impact_pairs_dashboard_path_collects_only_datasets() -> None:
+ """The batched pre-query only needs ``(dataset_id, tx)`` pairs.
+ Chart-related and self records aren't relevant."""
+ records = [
+ {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100},
+ {"entity_kind": "dataset", "entity_id": 7, "transaction_id": 200},
+ {"entity_kind": "chart", "entity_id": 9, "transaction_id": 300},
+ {"entity_kind": "dashboard", "entity_id": 1, "transaction_id": 400},
+ ]
+ assert _collect_impact_pairs(records, "Dashboard") == {(5, 100), (7, 200)}
+
+
+def test_collect_impact_pairs_dedupes_repeated_pairs() -> None:
+ """Multiple change records for the same (dataset, tx) collapse to
+ one pair — the batch query computes the count once."""
+ records = [
+ {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100},
+ {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100},
+ {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100},
+ ]
+ pairs = _collect_impact_pairs(records, "Dashboard")
+ assert pairs == {(5, 100)}
+
+
+def test_collect_impact_pairs_chart_path_returns_empty() -> None:
+ """Chart paths have no dashboard layer to count siblings on, so the
+ batch never needs to fire."""
+ records = [
+ {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100},
+ ]
+ assert _collect_impact_pairs(records, "Slice") == set()
+
+
+def test_collect_impact_pairs_dataset_path_returns_empty() -> None:
+ records = [
+ {"entity_kind": "chart", "entity_id": 5, "transaction_id": 100},
+ ]
+ assert _collect_impact_pairs(records, "SqlaTable") == set()
+
+
+def test_collect_impact_pairs_empty_records_returns_empty() -> None:
+ assert _collect_impact_pairs([], "Dashboard") == set()
# ---- parse_activity_query_params (shared API helper) ---------------------