mikebridge commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4031489900
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -372,53 +374,105 @@ def _repeats_an_earlier_block(
survivor while its streak is current. It is also exempt from operational
age-out (see :func:`_operational_candidates`), so an operator force-purge
block is retained permanently — never pruned by either category.
+
+ P is the immediately preceding distinct blocked timestamp. A row repeats
+ only when P is in the current streak and both timestamp groups contain
+ solely its reason (including all-NULL groups). LAG over timestamp groups
+ supplies P and its reason counts without a group self-join. The sc-120493
+ PostgreSQL round-1 plan materialized a groups CTE and joined on entity
+ alone before filtering ranks, comparing 36 million row pairs.
+
+ Keep the repeat-id query uncorrelated: sc-120493 measurements in
+ lock-hold-evidence/REPORT.md, Variant 2, showed MySQL repeatedly executing
+ per-row predecessor scalars. During re-check, scope blocked rows, timestamp
+ groups and boundaries with literal entity-type and UUID lists from the
+ fresh locked lookup. Their cross-product may include extra entity
histories,
+ but candidacy remains restricted to the discovered ids and checked in SQL.
"""
- earlier: sa.FromClause = table.alias("earlier_block")
- between: sa.FromClause = table.alias("reason_change")
- reason_changed_between: sa.ColumnElement[bool] = sa.exists(
- sa.select(sa.literal(1))
- .select_from(between)
- .where(
- sa.and_(
- between.c.status == STATUS_BLOCKED,
- between.c.entity_type == table.c.entity_type,
- between.c.entity_uuid == table.c.entity_uuid,
- # Inclusive bounds: a differing-reason block sharing an
- # exact timestamp with either endpoint still breaks the run,
- # so a reason-transition row tied with a neighbour is
- # preserved as a run head rather than pruned as a repeat
- # (the same preserving-side tie rule the pending and evidence
- # guards use). Inclusive bounds only ever add boundaries —
- # i.e. only ever preserve more, never delete more.
- between.c.created_on >= earlier.c.created_on,
- between.c.created_on <= table.c.created_on,
- between.c.reason.is_distinct_from(table.c.reason),
- )
+ source: sa.Table = PurgeAuditLog.__table__
+ scope: list[sa.ColumnElement[bool]] = []
+ if scope_entities is not None:
+ types: list[str] = sorted({t for t, _ in scope_entities})
+ uuids: list[str] = sorted({u for _, u in scope_entities if u is not
None})
+ scope = [source.c.entity_type.in_(types),
source.c.entity_uuid.in_(uuids)]
+ blocked_filters: list[sa.ColumnElement[bool]] = [
+ source.c.status == STATUS_BLOCKED,
+ source.c.entity_uuid.is_not(None),
+ source.c.created_on <= now,
+ ]
+ blocked: sa.Subquery = (
+ sa.select(
+ source.c.id,
+ source.c.entity_type,
+ source.c.entity_uuid,
+ source.c.created_on,
+ source.c.reason,
+ source.c.trigger,
)
- .correlate(table, earlier)
+ .select_from(source)
+ .where(*blocked_filters, *scope)
+ .correlate(None)
+ .subquery("blocked_rows")
)
- repeats: sa.ColumnElement[bool] = sa.exists(
- sa.select(sa.literal(1))
- .select_from(earlier)
+ groups: sa.Subquery = (
+ sa.select(
+ source.c.entity_type,
+ source.c.entity_uuid,
+ source.c.created_on.label("ts"),
+ sa.func.count().label("n"),
+ sa.func.count(source.c.reason).label("n_coded"),
+ sa.func.min(source.c.reason).label("min_reason"),
+ sa.func.max(source.c.reason).label("max_reason"),
+ )
+ .select_from(source)
+ .where(*blocked_filters, *scope)
+ .group_by(source.c.entity_type, source.c.entity_uuid,
source.c.created_on)
+ .correlate(None)
+ .subquery("blocked_timestamp_groups")
+ )
+ grp: sa.Subquery = (
+ sa.select(
+ groups,
+ *[
+ sa.func.lag(groups.c[name])
Review Comment:
Addressed in 6d41ae49f4871305726ffc42f284d2b126df58ea: dispatch retains the
legacy predicate for MySQL below 8.0 and older/unknown MySQL-family versions,
including normalized MariaDB detection. Both paths have candidate gates and
differential coverage. Fresh tests: 147 unit; 34 integration plus one skip on
SQLite; all 35 on PostgreSQL 17. No live MySQL 5.7 or MariaDB execution in this
round, so that validation remains a stated limitation. Pruning still defaults
off.
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -88,11 +88,10 @@
into a survivor fails the re-check and is not deleted). The re-check is a
SELECT
rather than a correlated ``WHERE`` on the DELETE because MySQL rejects a DELETE
whose subquery reads the target table (ERROR 1093); the DELETE names only the
-literal surviving ids. Because the re-check predicates are correlated
-(per-entity index probes) and scoped to at most the configured batch of ids,
-the locked work is bounded by the batch size times the history depth of the
-entities in it rather than by the table's size — a workload-dependent cost,
-not a time bound, which the batch-size setting trades against drain speed.
+literal surviving ids. The repeat check computes timestamp groups for the
+batch's entities in uncorrelated window tables; the other guards use
+correlated index probes. Lock-hold depends on the batch's entity histories,
Review Comment:
Addressed with 6d41ae49f4871305726ffc42f284d2b126df58ea and the updated PR
title/body. Removed the O(batch) guarantee and stale
500-row/short-indexed-window guidance. The default is 50, maximum 100, and lock
time remains dependent on history, plan and backend; no production capacity
claim is inferred from synthetic measurements.
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -372,53 +374,105 @@ def _repeats_an_earlier_block(
survivor while its streak is current. It is also exempt from operational
age-out (see :func:`_operational_candidates`), so an operator force-purge
block is retained permanently — never pruned by either category.
+
+ P is the immediately preceding distinct blocked timestamp. A row repeats
+ only when P is in the current streak and both timestamp groups contain
+ solely its reason (including all-NULL groups). LAG over timestamp groups
+ supplies P and its reason counts without a group self-join. The sc-120493
+ PostgreSQL round-1 plan materialized a groups CTE and joined on entity
+ alone before filtering ranks, comparing 36 million row pairs.
+
+ Keep the repeat-id query uncorrelated: sc-120493 measurements in
+ lock-hold-evidence/REPORT.md, Variant 2, showed MySQL repeatedly executing
+ per-row predecessor scalars. During re-check, scope blocked rows, timestamp
+ groups and boundaries with literal entity-type and UUID lists from the
+ fresh locked lookup. Their cross-product may include extra entity
histories,
+ but candidacy remains restricted to the discovered ids and checked in SQL.
"""
- earlier: sa.FromClause = table.alias("earlier_block")
- between: sa.FromClause = table.alias("reason_change")
- reason_changed_between: sa.ColumnElement[bool] = sa.exists(
- sa.select(sa.literal(1))
- .select_from(between)
- .where(
- sa.and_(
- between.c.status == STATUS_BLOCKED,
- between.c.entity_type == table.c.entity_type,
- between.c.entity_uuid == table.c.entity_uuid,
- # Inclusive bounds: a differing-reason block sharing an
- # exact timestamp with either endpoint still breaks the run,
- # so a reason-transition row tied with a neighbour is
- # preserved as a run head rather than pruned as a repeat
- # (the same preserving-side tie rule the pending and evidence
- # guards use). Inclusive bounds only ever add boundaries —
- # i.e. only ever preserve more, never delete more.
- between.c.created_on >= earlier.c.created_on,
- between.c.created_on <= table.c.created_on,
- between.c.reason.is_distinct_from(table.c.reason),
- )
+ source: sa.Table = PurgeAuditLog.__table__
+ scope: list[sa.ColumnElement[bool]] = []
+ if scope_entities is not None:
+ types: list[str] = sorted({t for t, _ in scope_entities})
+ uuids: list[str] = sorted({u for _, u in scope_entities if u is not
None})
+ scope = [source.c.entity_type.in_(types),
source.c.entity_uuid.in_(uuids)]
+ blocked_filters: list[sa.ColumnElement[bool]] = [
+ source.c.status == STATUS_BLOCKED,
+ source.c.entity_uuid.is_not(None),
+ source.c.created_on <= now,
+ ]
+ blocked: sa.Subquery = (
+ sa.select(
+ source.c.id,
+ source.c.entity_type,
+ source.c.entity_uuid,
+ source.c.created_on,
+ source.c.reason,
+ source.c.trigger,
)
- .correlate(table, earlier)
+ .select_from(source)
+ .where(*blocked_filters, *scope)
+ .correlate(None)
+ .subquery("blocked_rows")
)
- repeats: sa.ColumnElement[bool] = sa.exists(
- sa.select(sa.literal(1))
- .select_from(earlier)
+ groups: sa.Subquery = (
+ sa.select(
+ source.c.entity_type,
+ source.c.entity_uuid,
+ source.c.created_on.label("ts"),
+ sa.func.count().label("n"),
+ sa.func.count(source.c.reason).label("n_coded"),
+ sa.func.min(source.c.reason).label("min_reason"),
+ sa.func.max(source.c.reason).label("max_reason"),
+ )
+ .select_from(source)
+ .where(*blocked_filters, *scope)
+ .group_by(source.c.entity_type, source.c.entity_uuid,
source.c.created_on)
+ .correlate(None)
+ .subquery("blocked_timestamp_groups")
+ )
+ grp: sa.Subquery = (
+ sa.select(
+ groups,
+ *[
+ sa.func.lag(groups.c[name])
+ .over(
+ partition_by=(groups.c.entity_type, groups.c.entity_uuid),
Review Comment:
Addressed in 6d41ae49f4871305726ffc42f284d2b126df58ea: a
shared-UUID/different-entity-type fixture exercises both repeat
implementations, alongside the expanded equivalence cases. The partition
expression remains unchanged. Fresh affected suites passed, including all 35
integration tests on disposable PostgreSQL 17.
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -623,7 +725,10 @@ def _evidence_candidates(now: datetime, cutoff: datetime,
limit: int) -> sa.sql.
def _evidence_predicates(
- table: sa.FromClause, now: datetime, cutoff: datetime
+ table: sa.FromClause,
+ now: datetime,
+ cutoff: datetime,
+ scope_entities: Sequence[tuple[str, str | None]] | None = None,
Review Comment:
Addressed in 6d41ae49f4871305726ffc42f284d2b126df58ea. Discovery carries
immutable entity pairs before the lock; evidence pruning does not fetch or
consume unused scope, while its locked ID candidacy recheck remains. Explicit
recheck adapters preserve argument checking. Fresh tests and changed-file
pre-commit passed.
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -372,53 +374,105 @@ def _repeats_an_earlier_block(
survivor while its streak is current. It is also exempt from operational
age-out (see :func:`_operational_candidates`), so an operator force-purge
block is retained permanently — never pruned by either category.
+
+ P is the immediately preceding distinct blocked timestamp. A row repeats
+ only when P is in the current streak and both timestamp groups contain
+ solely its reason (including all-NULL groups). LAG over timestamp groups
+ supplies P and its reason counts without a group self-join. The sc-120493
+ PostgreSQL round-1 plan materialized a groups CTE and joined on entity
+ alone before filtering ranks, comparing 36 million row pairs.
+
+ Keep the repeat-id query uncorrelated: sc-120493 measurements in
+ lock-hold-evidence/REPORT.md, Variant 2, showed MySQL repeatedly executing
Review Comment:
Addressed in 6d41ae49f4871305726ffc42f284d2b126df58ea: removed the
unavailable repository-relative report link and retained the reason for keeping
the repeat-ID query uncorrelated. Final-snapshot independent review found no
publication-blocking defects; the documented backend/capacity limitations
remain.
--
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]
