(superset) 16/23: refactor(activity-view): SQL review tidies — mappings + perf-seed docstring

[kg...@apache.org]

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 3a9f863194347e7becfc0fdf87076875133963d0
Author: Mike Bridge <mi...@ext.preset.io>
AuthorDate: Tue May 26 16:43:39 2026 -0600

    refactor(activity-view): SQL review tidies — mappings + perf-seed docstring
    
    Two structural changes from the second SQLAlchemy review. No behaviour
    change; full activity-view suite (70 unit + 30 integration) still
    green.
    
    * Tidy 1 (Warning #2): switch _batch_chart_counts and
      _batch_datasets_used_by_charts from positional row-unpack to
      .mappings()-keyed access. Column-rename-safe — a future edit that
      reorders columns in the SELECT can't silently misassign values
      during the Python loop. Matches the existing convention in
      _select_change_rows_for_kinds.
    
    * Tidy 2 (Suggestion #1): docstring on _seed_activity_history
      explaining why it intentionally commits without rollback (unlike
      the T053 convention in activity_view_tests.py). The seed IS the
      setup, not the unit under test — the endpoint reads a realistic
      post-commit state of the shadow tables.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <no...@anthropic.com>
---
 superset/versioning/activity.py                    | 22 +++++++++++++++-------
 .../versioning/perf_validation_tests.py            | 11 +++++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/superset/versioning/activity.py b/superset/versioning/activity.py
index fb673189e5c..03542cd934d 100644
--- a/superset/versioning/activity.py
+++ b/superset/versioning/activity.py
@@ -233,11 +233,14 @@ def _batch_datasets_used_by_charts(
                 slices_tbl.c.datasource_id.is_not(None),
             )
         )
+        .mappings()
         .all()
     )
     grouped: dict[int, list[tuple[int, Window]]] = {}
-    for slice_id, dataset_id, start_tx, end_tx in rows:
-        grouped.setdefault(slice_id, []).append((dataset_id, (start_tx, end_tx)))
+    for row in rows:
+        grouped.setdefault(row["id"], []).append(
+            (row["datasource_id"], (row["transaction_id"], row["end_transaction_id"]))
+        )
     return grouped
 
 
@@ -593,7 +596,7 @@ def _batch_chart_counts(
         slices_tbl.c.datasource_type == "table",
         slices_tbl.c.operation_type != 2,
     )
-    rows = db.session.connection().execute(stmt).all()
+    rows = db.session.connection().execute(stmt).mappings().all()
 
     # For each pair, collect the slice_ids whose two validity windows
     # both straddle target_tx. ``set`` dedupes within a pair.
@@ -602,12 +605,17 @@ def _batch_chart_counts(
     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 row in rows:
+        ds_id = row["datasource_id"]
         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)
+            in_m2m = row["m2m_start"] <= target_tx and (
+                row["m2m_end"] is None or row["m2m_end"] > target_tx
+            )
+            in_slice = row["slice_start"] <= target_tx and (
+                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(slice_id)
+                matches.setdefault((ds_id, target_tx), set()).add(row["slice_id"])
 
     return {pair: len(slice_ids) for pair, slice_ids in matches.items()}
 
diff --git a/tests/integration_tests/versioning/perf_validation_tests.py b/tests/integration_tests/versioning/perf_validation_tests.py
index fa4d9cea257..5e792db50c0 100644
--- a/tests/integration_tests/versioning/perf_validation_tests.py
+++ b/tests/integration_tests/versioning/perf_validation_tests.py
@@ -289,6 +289,17 @@ class PerfValidationTests(SupersetTestCase):
         spanning all three entity kinds — enough to exercise the
         decoration, visibility, and impact-batch paths without needing a
         multi-dataset fixture builder. Returns the dashboard UUID.
+
+        **Why this commits without rollback** (unlike the test bodies in
+        ``activity_view_tests.py``): the whole point of a perf seed is
+        that the rows it produces have actually been persisted, so the
+        endpoint hit that follows reads a realistic state of the
+        ``version_changes`` / shadow tables. T053's
+        ``try/finally``+``rollback`` convention is for tests that
+        assert on *which records were captured*; here the seed IS the
+        setup, not the unit under test. The fixture's session-scoped
+        ``_cleanup`` removes the dashboard / slices at session teardown,
+        which is when the shadow rows age out too.
         """
         # pylint: disable=import-outside-toplevel
         from superset.connectors.sqla.models import SqlaTable