aminghadersohi commented on code in PR #44349:
URL: https://github.com/apache/superset/pull/44349#discussion_r4032586800
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -372,53 +374,139 @@ 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.
+
+ All five ``LAG`` columns share one SQL-level named ``WINDOW w`` (a raw
+ text fragment: SQLAlchemy Core has no construct for a named ``WINDOW``
+ clause) instead of five separate ``LAG(...) OVER (...)`` expressions that
+ happen to repeat the same partition/order spec. PostgreSQL already
+ recognizes five identical inline window specs as one logical pass, but
+ MySQL 8 does not merge them — each materializes its own temporary table,
+ roughly five sequential passes over the batch's timestamp groups. A named
+ window is the SQL-level way to say "this is the same window" so MySQL
+ evaluates it once; PostgreSQL and SQLite (>= 3.25) accept the same syntax
+ unchanged (sc-120950).
+ An equality join back to a *second* instance of the timestamp-groups
+ derived table (fetching P's aggregates via
+ ``(entity_type, entity_uuid, ts = prev_ts)`` instead of four more ``LAG``
+ columns) was measured and rejected here: PostgreSQL's planner
+ misestimates the derived table's row count for a single-entity batch scope
+ and chooses a Nested Loop over an unindexed ``Materialize`` of the second
+ instance — an O(batch × history) comparison, the same failure shape as
+ the sc-120493 round-1 CTE/entity-only-merge regression, just via a
+ different join path. The named-``WINDOW`` shape keeps the exact join
+ structure already measured safe on PostgreSQL (sc-120493): only the
+ ``LAG`` columns' SQL text changes.
+
+ 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)]
Review Comment:
The cross-product here is bounded to the in-scope entities; it does not
admit unrelated histories.
Emitted predicate, identical in all three derived tables and on both
PostgreSQL and MySQL:
```sql
WHERE purge_audit_log.status = 'blocked'
AND purge_audit_log.entity_uuid IS NOT NULL
AND purge_audit_log.created_on <= :now
AND purge_audit_log.entity_type IN ('dashboards', 'slices', 'tables')
AND purge_audit_log.entity_uuid IN (...)
```
Two bounds:
1. `entity_type` is the table name of a registered `SoftDeleteMixin`
subclass. That universe is exactly three values — `dashboards`, `slices`,
`tables`. With `MAX_BATCH_SIZE = 500` the combination count tops out at 3 × 500
= 1,500. Reaching 250,000 would require 500 distinct entity types.
2. The two `IN` terms are ANDed, so the matched set is their intersection.
`entity_uuid` is the entity's own `uuid4`, so it identifies exactly one entity
and therefore one `entity_type`. `entity_uuid IN (≤500 uuids)` alone already
bounds the scan to the in-scope entities' histories; the additional (type,
uuid) combinations match zero rows, because `entity_type IN (...)` can only
narrow the set, never widen it.
Measured on a heterogeneous batch — 3 entity types × 4 entities × 3 history
rows in scope, plus 150 out-of-scope same-type histories present in the table:
```
scope pairs : 12
types x uuids combos : 3 x 12 = 36
rows matched cross-product : 36
rows matched paired : 36
identical row sets : True
extra rows from cross-prod : 0
```
`WHERE (entity_type, entity_uuid) IN (...)` selects the same rows. The
existing conjunction also matches the leading columns of
`ix_purge_audit_log_pruning (status, entity_type, entity_uuid, created_on)`
directly.
--
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]
