aminghadersohi commented on code in PR #43837:
URL: https://github.com/apache/superset/pull/43837#discussion_r3931305240
##########
superset/versioning/activity/queries.py:
##########
@@ -118,14 +119,66 @@ def first_tracked_tx(
# ---- Phase A: relationship-traversal queries ------------------------------
+# ``operation_type`` values on a Continuum association shadow row
+# (sqlalchemy_continuum.operation.Operation): INSERT attaches, DELETE detaches.
+# UPDATE never occurs for a pure M2M association (there is nothing to update on
+# a (dashboard, slice) pair); if it ever appeared it is ignored — neither
+# opening nor closing a window — so an open attachment simply continues.
+# These mirror the library enum's numeric values;
``test_m2m_op_constants_match_
+# continuum`` pins them so a Continuum renumber fails loudly rather than
silently.
+_M2M_OP_INSERT = 0
+_M2M_OP_DELETE = 2
+
+
+def _attachment_windows(
+ rows: list[tuple[int, int, int]],
+) -> list[tuple[int, Window]]:
+ """Pair INSERT / DELETE association-version rows into ``[attach, detach)``
+ windows, one per attachment episode.
+
+ Each row is ``(slice_id, transaction_id, operation_type)``. Continuum
+ **never closes** an association shadow row's ``end_transaction_id`` — its
+ unit-of-work only *inserts* association versions
+ (``create_association_versions``); the validity backfill that sets
+ ``end_transaction_id`` runs for parent objects, not for M2M links. So the
+ detach boundary lives on the DELETE row's ``transaction_id``, not on the
+ attach row's ``end_transaction_id`` (which stays NULL for the association's
+ whole life). An INSERT opens a window; the next DELETE closes it at its
+ transaction id; an attachment with no following DELETE stays open (the
+ chart is still on the dashboard). A DELETE at the same transaction as its
+ open (add-and-remove in one save) yields no window — the chart was never
+ on a committed dashboard state.
+ """
+ result: list[tuple[int, Window]] = []
+ # operation_type is part of the sort key so that, within one transaction,
+ # INSERT (0) sorts before DELETE (2): an add-and-remove in a single save is
+ # then seen open-before-close and collapses to no window (the DELETE finds
+ # ``tx == open_tx``, not ``>``). Do not drop it from the key.
+ rows_sorted = sorted(rows, key=lambda r: (r[0], r[1], r[2]))
+ for slice_id, group in groupby(rows_sorted, key=lambda r: r[0]):
+ open_tx: int | None = None
+ for _slice_id, tx, operation_type in group:
+ if operation_type == _M2M_OP_DELETE:
+ if open_tx is not None and tx > open_tx:
+ result.append((slice_id, Window(open_tx, tx)))
+ open_tx = None
+ elif operation_type == _M2M_OP_INSERT and open_tx is None:
Review Comment:
Question on the `and open_tx is None` guard, prompted by bito's note — I
think it is a real edge case, so here it is with the trace.
The sort key puts INSERT(0) before DELETE(2) within a transaction, which
makes add-and-remove-in-one-save collapse correctly (your test covers it). But
the mirror case — *remove and re-add* in one save, while the chart was already
attached — goes wrong:
```
rows = [(7, 3, 0), (7, 5, 2), (7, 5, 0)] # attached@3; removed+re-added@5
sorted = [(7, 3, 0), (7, 5, 0), (7, 5, 2)]
open_tx = 3 # INSERT@3
INSERT@5 -> ignored # open_tx is not None
DELETE@5 -> 5 > 3 # emits Window(3, 5), open_tx = None
result: [(7, Window(3, 5))]
```
The chart is still on the dashboard, but the window is closed at 5, so every
later edit drops out of `_resolve_dashboard_scope`. That is this PR's bug
inverted, and it is a behavioural change: the old code left the window open
here (accidentally, but with the right outcome).
How reachable is it? I could not construct a concrete save path that emits
this ordering — `dashboard.slices = current_slices` (`daos/dashboard.py:374`)
is a wholesale reassignment whose collection diff nets an unchanged member out
to no statement at all. But the schema explicitly anticipates the same-tx pair:
`operation_type` is in the primary key precisely for it, and the migration says
so in as many words (`56cd24c07170`, lines 512-515) — *"a single transaction
can in principle produce both INSERT and DELETE shadows for the same
(dashboard_id, slice_id) pair"*.
The genuine difficulty is that `{INSERT@t, DELETE@t}` is ambiguous in
isolation: add-then-remove and remove-then-re-add produce identical rows. But
`open_tx` disambiguates it — if a window is already open, an INSERT at the same
tx can only be a re-add, since you cannot attach something already attached. So
something like:
```python
elif operation_type == _M2M_OP_INSERT and (open_tx is None or ...):
```
keeping the window open when the same-tx INSERT follows a DELETE. Entirely
your call whether it is worth handling versus documenting as out of scope — I
would be happy with a comment saying the ordering is deliberate and why.
--
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]
