mikebridge commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4020983777
##########
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:
Good catch on the documented MySQL 5.7 compatibility gap; could we preserve
the non-window path for that backend? Pruning does default to disabled
(`PURGE_AUDIT_PRUNING_ENABLED = False`), so this affects deployments that
enable it rather than all default deployments.
##########
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:
Good catch—the source already describes history-dependent lock-hold, and the
title overstates the guarantee. Would it be worth dropping the O(batch) claim
and describing the candidate-entity scoping instead?
##########
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:
Good catch—the partition is correct, but the fixtures do not pin the
shared-UUID/different-type case. Could we add an executable case that preserves
each type’s first block and fails when entity_type is removed from the
partition?
##########
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:
Good catch—the evidence predicate does not consume the entity scope. Could
we make the locked lookup category-specific while retaining evidence’s locked
ID recheck and keeping scope lookup for duplicate and operational pruning?
##########
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:
Good catch—the relative path is not present in this repository. Could we
take your suggestion, which keeps the reason for the uncorrelated query without
pointing maintainers at an unavailable local artifact?
--
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]
