mikebridge commented on code in PR #44251:
URL: https://github.com/apache/superset/pull/44251#discussion_r4031543158
##########
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:
Good catch—addressed in 7797be4e014d21e82cfc724fd430ab31882ac5e1 with the
exact UPDATE row created at the target transaction. The real negative control
(`<=` changed to `<`) failed that case; restoring the inclusive predicate
passed all 180 versioning unit tests. Isolated SQLite integration: 9 passed, 1
server-dialect locking test skipped. Current-head CI is still running.
##########
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:
Good catch—7797be4e014d21e82cfc724fd430ab31882ac5e1 documents all three
indistinguishable shapes, including a surviving re-insertion INSERT after
erased birth/covering history, and adds a characterization pin. It also
strengthens foreign-ID coverage to a target after the closer, with the foreign
incarnation removed. The ratified guard is unchanged; dependency-closure
retention stays separate and post-GA in sc-120945. The PR summary has been
aligned with that limitation.
--
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]
