fitzee commented on code in PR #44089:
URL: https://github.com/apache/superset/pull/44089#discussion_r4001460291
##########
superset/versioning/changes/shadow_queries.py:
##########
@@ -233,54 +236,73 @@ def _affected_dashboard_ids_at_tx(session: Session, tx:
int) -> set[int]:
def _dashboard_slice_uuids_at_tx(
- session: Session, dashboard_id: int, tx: int
+ session: Session, attached: list[tuple[int, Window]], tx: int
) -> list[str]:
- """Slice UUIDs attached to *dashboard_id* as of *tx*, read by joining
- ``dashboard_slices_version`` (M2M membership) against
- ``slices_version`` (slice content).
-
- Joining through both is necessary — and matches the same query
- Continuum's M2M ``Reverter`` uses — because a slice that's
- referenced by the M2M but has no slice-version row at this tx is
- treated as "not yet versioned" and excluded.
+ """Return the uuids of charts attached to a dashboard at *tx*.
+
+ *attached* is
:func:`~superset.versioning.membership.charts_attached_to_dashboard`'s
+ output — ``(slice_id, window)`` pairs where each window is the chart's
+ INSERT/DELETE-paired ``[attach, detach)`` interval — so membership at *tx*
+ is simply the charts whose window contains *tx*. The raw association-shadow
+ validity predicate (``end_transaction_id IS NULL OR > tx`` +
+ ``operation_type != DELETE``) must NOT be used: Continuum never closes
+ ``end_transaction_id`` on the M2M association shadow, so a chart attached
at
+ tx 1 and removed at tx 5 would still read as a member at tx 10 —
+ over-reporting membership in the change-record diff (sc-120007, the third
+ consumer of this pattern; restore and impact were fixed the same way in
+ #44010 / #43837).
+
+ The ``slices_version`` (content) shadow, by contrast, *does* close
+ ``end_transaction_id`` correctly, so its validity predicate is trustworthy
+ and is kept: a chart attached at *tx* but with no slice-version row at *tx*
+ is "not yet versioned" and excluded, matching Continuum's M2M ``Reverter``.
+ The read runs on the passed *session* — the committing connection whose
+ flushed-but-uncommitted current-tx rows are visible only there.
Returns UUIDs (strings) so the result can be diffed by the existing
:func:`diff_dashboard_slices` helper, which keys on uuid.
"""
+ attached_ids = {slice_id for slice_id, window in attached if
window.contains(tx)}
+ if not attached_ids:
+ return []
+
# pylint: disable=import-outside-toplevel
+ # Deferred imports: ``version_class`` / ``Slice`` because this module loads
+ # from ``init_versioning()`` before all mappers are configured (see the
+ # module docstring); ``activity.kinds`` because it imports
+ # ``ENTITY_KIND_BY_CLASS_NAME`` from ``superset.versioning.changes``, so a
+ # module-top import would cycle back into this package during the
+ # changes-listener bootstrap (activity.kinds → changes → listener →
+ # shadow_queries).
from sqlalchemy_continuum import version_class
from superset.models.slice import Slice
+ from superset.versioning.activity.kinds import chunked_ids,
ENTITY_ID_CHUNK_SIZE
- metadata = version_class(Slice).__table__.metadata
- m2m_tbl = metadata.tables.get("dashboard_slices_version")
+ # Resolve each attached chart's uuid from the content shadow at tx. The id
+ # set is a dashboard's simultaneous membership at one tx (realistically
+ # dozens), but chunk the IN anyway to stay under SQLite's bind-variable
+ # floor, mirroring the sibling impact rollup (#44010).
slices_tbl = version_class(Slice).__table__
- if m2m_tbl is None:
- return []
-
- rows = (
- session.connection()
- .execute(
- sa.select(slices_tbl.c.uuid).where(
- slices_tbl.c.id == m2m_tbl.c.slice_id,
- m2m_tbl.c.dashboard_id == dashboard_id,
- m2m_tbl.c.transaction_id <= tx,
- sa.or_(
- m2m_tbl.c.end_transaction_id.is_(None),
- m2m_tbl.c.end_transaction_id > tx,
- ),
- m2m_tbl.c.operation_type != OPERATION_DELETE,
- slices_tbl.c.transaction_id <= tx,
- sa.or_(
- slices_tbl.c.end_transaction_id.is_(None),
- slices_tbl.c.end_transaction_id > tx,
- ),
- slices_tbl.c.operation_type != OPERATION_DELETE,
+ uuids: list[str] = []
+ for chunk in chunked_ids(attached_ids, ENTITY_ID_CHUNK_SIZE):
+ rows = (
+ session.connection()
+ .execute(
+ sa.select(slices_tbl.c.uuid).where(
+ slices_tbl.c.id.in_(chunk),
+ slices_tbl.c.transaction_id <= tx,
Review Comment:
**Minor (DRY):** this three-line Continuum validity clause (`transaction_id
<= tx` AND `end_transaction_id IS NULL OR > tx` AND `operation_type != DELETE`)
is re-implemented inline while `shadow_rows_valid_at` (line 53, same module)
already encodes exactly these semantics. They can't be unified as-is —
`shadow_rows_valid_at` takes a single `fk_col == fk_value` and returns full
rows, whereas this needs `id.in_(chunk)` selecting only `uuid` — so this is a
real cost, not a trivial swap. Still, a future change to Continuum validity
semantics now has to be edited in both places; worth a shared predicate helper
(returning just the `and_(...)` clause) that both call sites compose.
##########
superset/versioning/changes/shadow_queries.py:
##########
@@ -317,8 +340,14 @@ def _dashboard_child_records_for_tx_from_shadows(
if prior_tx is None:
continue
- post_uuids = _dashboard_slice_uuids_at_tx(session, dashboard_id,
transaction_id)
- pre_uuids = _dashboard_slice_uuids_at_tx(session, dashboard_id,
prior_tx)
+ # Resolve the attachment windows once (threading the committing
+ # *session* so the flushed-but-uncommitted current-tx association rows
+ # are visible), then take the pre/post membership by which windows
+ # contain each tx — the windows are tx-independent, so no need to
+ # re-scan the association history for both reads.
+ attached = charts_attached_to_dashboard(dashboard_id, session=session)
+ post_uuids = _dashboard_slice_uuids_at_tx(session, attached,
transaction_id)
+ pre_uuids = _dashboard_slice_uuids_at_tx(session, attached, prior_tx)
Review Comment:
**Latent (pre-existing) content-validity asymmetry that this rewrite
preserves — can emit a phantom add/remove.** `pre_uuids` and `post_uuids` are
each gated on the chart having a *valid `slices_version` (content) row* at
their respective tx, independently. So a chart whose attachment window spans
both `prior_tx` and `transaction_id`, but whose content shadow is valid at only
one of them, diffs as an add or remove even though its membership never changed.
Concrete: chart X attached at tx=2 (window `[2, None)`) but its first
`slices_version` row is at tx=3; a title edit lands at tx=3 so `prior_tx=2`.
`post(tx=3)`: window contains 3 ✓ and content valid at 3 ✓ → X in post.
`pre(tx=2)`: window contains 2 ✓ but content `transaction_id=3 > 2` → X
excluded from pre. `diff_dashboard_slices` then emits a spurious *"chart
added"* for a chart that was already a member — the change record misreports
"content became versioned" as "membership changed."
This is not introduced here (the old joined query gated on content-validity
too), but the function is being rewritten, so it's the natural moment to decide
whether membership diffing should depend on content-validity at all, or only on
the attachment window. Flagging as latent; not a blocker.
--
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]
