aminghadersohi commented on code in PR #44251:
URL: https://github.com/apache/superset/pull/44251#discussion_r4026294479
##########
tests/unit_tests/versioning/test_restore.py:
##########
@@ -131,3 +131,205 @@ def test_single_flush_scope_skips_flush_on_exception() ->
None:
with single_flush_scope(session):
raise RuntimeError("boom")
session.flush.assert_not_called()
+
+
+class _Row:
+ def __init__(self, tx: int, end: int | None, op: int) -> None:
+ self.transaction_id = tx
+ self.end_transaction_id = end
+ self.operation_type = op
+
+
+def _provable(rows: list[_Row], target_tx: int) -> bool:
+ from superset.versioning.restore import _child_state_provable_at
+
+ return _child_state_provable_at(rows, target_tx)
+
+
+_INSERT, _UPDATE, _DELETE = 0, 1, 2
+
+
[email protected](
+ ("rows", "target_tx", "expected", "case"),
+ [
+ # A surviving closed terminal DELETE proves absence
+ # UNCONDITIONALLY (ratified, sc-120012): retention cannot erase
+ # its closer without erasing the DELETE row itself (the pruner's
+ # close-tx predicate), and purge never touches the live parent's
+ # rows — so the missing closer is a purged foreign incarnation
+ # of a recycled id. This was the #44251 CI false-refusal class.
+ (
+ [_Row(2, 5, _INSERT), _Row(5, 8, _DELETE)],
+ 10,
+ True,
+ "closed terminal delete is provable absence",
+ ),
+ # Ping-pong recycling: the pk went foreign and came BACK after
+ # the target; the last same-parent row at/before the target is
+ # the DELETE — absent, regardless of the later re-birth.
+ (
+ [
+ _Row(2, 5, _INSERT),
+ _Row(5, 8, _DELETE),
+ _Row(15, 20, _INSERT),
+ _Row(20, None, _DELETE),
+ ],
+ 10,
+ True,
+ "ping-pong: target inside the foreign period is absent",
+ ),
+ # The guard's core case stays closed: a non-DELETE row whose
+ # interval expired before the target means its same-parent
+ # successor was pruned (close-tx pruned, create-tx kept) — the
+ # child may have existed at the target.
+ (
+ [_Row(2, 5, _INSERT), _Row(5, 8, _DELETE), _Row(9, 10, _UPDATE)],
+ 10,
+ False,
+ "expired non-delete last row refuses",
+ ),
+ ],
+)
+def test_child_state_absence_and_refusal_rules(
+ rows: list[_Row],
+ target_tx: int,
+ expected: bool,
+ case: str,
+) -> None:
+ """sc-120012 ratified semantics after the #44251 CI rounds: closed
+ terminal DELETEs are absence; expired non-DELETE intervals refuse."""
+ from superset.versioning.restore import _child_state_provable_at
+
+ assert _child_state_provable_at(rows, target_tx) is expected, case
+
+
[email protected](
+ ("rows", "target_tx", "expected", "case"),
+ [
+ # A surviving non-DELETE row covers the target: complete.
+ ([_Row(5, None, _INSERT)], 10, True, "live row covers"),
+ ([_Row(5, 20, _UPDATE)], 10, True, "closed row covers"),
Review Comment:
No case pins `transaction_id == target_tx`. Changing the covering test's
`<=` to `<` passes all 24 unit and 9 integration tests, yet flips a column
edited in the very transaction being restored to from restorable to refused —
the guard's most common false-positive direction.
```suggestion
# A surviving non-DELETE row covers the target: complete.
([_Row(5, None, _INSERT)], 10, True, "live row covers"),
([_Row(5, 20, _UPDATE)], 10, True, "closed row covers"),
# Boundary: a row created AT the target tx covers it (half-open
# [tx, end)) — the child changed in the transaction restored to.
([_Row(10, None, _UPDATE)], 10, True, "row created at target
covers"),
```
##########
superset/versioning/restore.py:
##########
@@ -71,6 +72,194 @@
}
+class PrunedChildHistoryError(Exception):
+ """The target version's child history is no longer fully recoverable.
+
+ Version-history retention prunes closed child shadow rows (and their
+ ``version_transaction`` rows) once they age out; a restore that
+ proceeded anyway would persist an INCOMPLETE column/metric set for a
+ ``SqlaTable`` — a durable partial write. Restore fails closed
+ instead (sc-120012). The message is user-facing.
+ """
+
+ def __init__(self, model_name: str, detail: str) -> None:
+ super().__init__(
+ f"This {model_name} version can no longer be fully restored: "
+ f"{detail} needed by the snapshot were pruned by "
+ "version-history retention. The entity was left unchanged."
+ )
+
+
+def _verify_child_history_complete(entity: Any, target_tx: int) -> None:
+ """Refuse the restore when a needed child shadow row was pruned.
+
+ ``revert(relations=...)`` reconstructs a ``SqlaTable``'s columns and
+ metrics from the child shadow rows valid at *target_tx*. Retention
+ can have pruned exactly those rows while the parent's row at
+ *target_tx* survives; the pruner also deletes the covering
+ ``version_transaction`` rows (change records cascade with them), so
+ the only surviving evidence is the validity chain itself. Per child
+ (grouped by the child's own ``id``), the state at *target_tx* is
+ PROVABLE when a surviving row's validity interval covers it (a
+ non-DELETE covering row: present, restored; a DELETE covering row:
+ provably absent), or when every surviving row lies beyond the target
+ and the earliest is the child's birth INSERT (born after). See
+ :func:`_child_state_provable_at` for the interval semantics.
+
+ Anything else means a pruned row MAY have covered ``target_tx`` —
+ fail closed (sc-120012).
+
+ Known limitation (ratified, sc-120012): the fail-closed guard refuses
+ every DETECTABLE pruning of needed child history, including a pruned
+ closed row whose successor survives, and protects all restores
+ targeting versions within the retention window. One residual fails
Review Comment:
"One residual" undercounts — born-after fails open too. A same-parent
re-birth INSERT can survive while the birth and covering rows prune via their
close-tx, leaving a row set identical to the legitimate born-after case, so no
read-side check separates them. No fence: the wording is yours.
--
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]
