You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/09/14 21:07:44 UTC
[PR] perf(deletion-retention): compute the purge-audit repeat predicate once per entity set — lock-hold O(batch) on every backend [superset]
mikebridge opened a new pull request, #44262:
URL: https://github.com/apache/superset/pull/44262
### SUMMARY
Follow-up to #43490 (sadpandajoe's lock-hold thread, https://github.com/apache/superset/pull/43490#discussion_r3978079306). Tracked as sc-120493.
**Problem.** `_repeats_an_earlier_block` in `superset/commands/deletion_retention/prune_audit.py` was an `EXISTS` nested inside an `EXISTS`, correlated per candidate row — O(history²) per candidate — and the coordination-locked re-check in `_delete_batch` evaluates it for every id in the batch while `write_ahead()` waits. On a 6,000-row multi-reason entity a 500-id batch held the lock **6.4 s on PostgreSQL and ≈50 s on MySQL 8 under REPEATABLE READ** (production's effective level). #43490 shipped the batch-size knob as mitigation; this PR fixes the shape.
**Change.** The predicate is reformulated as an *uncorrelated* derived-table select and the row is tested with `id IN (...)`:
- `blocked_rows` — blocked, non-NULL-uuid, `created_on <= now` rows (force rows included: a force row may be the predecessor of a later repeat; it is only never a repeat itself);
- `blocked_timestamp_groups` — `GROUP BY entity_type, entity_uuid, created_on` with `COUNT(*)`, `COUNT(reason)`, `MIN(reason)`, `MAX(reason)`;
- `preceding_groups` — the group table plus `LAG(ts / n / n_coded / min_reason / max_reason) OVER (PARTITION BY entity ORDER BY ts)`: the immediately preceding *distinct* timestamp **P** and its reason statistics, with no group self-join;
- `repeat_boundary` — per-entity `MAX(created_on)` of streak-breaking rows.
A row repeats iff `trigger <> 'force'`, P exists, `P.ts > boundary` (or no boundary), both timestamp groups are reason-uniform (all-NULL or one shared code — the inclusive tie rule: a differing-reason row tied with either endpoint still breaks the run) and `P.reason IS NOT DISTINCT FROM row.reason`. This is the same rule as before: given an earlier same-reason block E with no reason change in `[E, R]`, `P.ts ∈ [E.ts, R.ts)` so P's group matches and `P > boundary` follows from `E > boundary`; conversely take `E = P`. The equivalence is asserted by execution, not argued: a 40-seed randomised equivalence test keeps the legacy nested-`EXISTS` predicate in the test module and asserts identical candidate sets for all three pruning categories (passes on PostgreSQL and MySQL). All pre-existing behavioural tests are unchanged.
**Scope under the lock.** `_delete_batch` reads the batch ids' distinct `(entity_type, entity_uuid)` pairs immediately after `acquire_coordination_lock` and binds them as literal `IN` lists inside the three derived tables. This is scoping only — the columns are immutable for a row, the lookup runs on the fresh post-lock snapshot, and every candidacy predicate is still re-verified in SQL by the re-check — so the sc-118200 survivor invariant is untouched; a unit test pins the order lock → scope lookup → re-check → delete → commit and that pairs ≤ ids ≤ `MAX_BATCH_SIZE`. Literal values are deliberate: a `DISTINCT … JOIN` or `IN (subquery)` scope collapsed PostgreSQL's cardinality estimate of the group table to 1 row and produced a nested loop with `created_on = ts` as a join *filter* (36M row pairs, 5.4 s), and a subquery scope was catastrophic on MySQL (>433 s). The `MAX_BATCH_SIZE` comment records the bind budget (~3,500 positional placeholders at the 500 ceiling;
SQLite ≥ 3.32's 32,766 default).
**Measurements** — coordination-lock hold, medians of 5, on a 10,805-row seed with one hot entity of 6,000 blocked rows in three reasons (five resolved streaks + one current), `ANALYZE` after seeding, MySQL session forced to `REPEATABLE READ`, a concurrent writer thread timing its own lock wait; the harness performs the scope lookup inside the locked window exactly as `_delete_batch` does. Candidate counts 500/500 and 50/50 on every run.
| round | shape | PG @500 | PG @50 | MySQL @500 | MySQL @50 |
|---|---|---|---|---|---|
| — | #43490 nested EXISTS | 6,423 ms | 149 | ≈49,700 | 1,243 |
| 1 | DENSE_RANK CTEs + group self-join, JOIN scope | 6,621 | 6,609 | 68 | 42 |
| 2 | LAG over group table, plain subqueries, JOIN scope | 5,392 | 52 | 155 | 122 |
| 3 | round 2 + literal entity scope (**this PR**) | **75** | **29** | **160** | **131** |
Writer wait ≈ lock-hold − 10 ms throughout. Unlocked discovery: PG 60 ms, MySQL 332 ms at batch 500 (was 1.4 s / 97 s). MySQL's remaining cost is five sequential window passes for the five `LAG` columns (SQLAlchemy Core cannot emit a named `WINDOW`; a single-pass raw prototype ran in 91 ms) — accepted for this PR, tracked as **follow-up sc-120950 for the single MySQL window pass**. Harness, per-round plans and raw outputs live in the spec repo under `reviews/2026-09-14-pr-43490/lock-hold-evidence/` (`sc-120493/`).
<details><summary>PostgreSQL 16 — locked re-check plan, batch 500 (abridged: costs, buffers and trivial filters stripped)</summary>
```
=== PLAN: locked re-check ===
Nested Loop Anti Join (actual time=20.347..61.103 rows=500 loops=1)
-> Nested Loop (actual time=20.345..60.760 rows=500 loops=1)
-> HashAggregate (actual time=20.149..20.251 rows=936 loops=1)
-> Merge Right Join (actual time=5.291..20.004 rows=936 loops=1)
Merge Cond: ((purge_audit_log_2.entity_uuid)::text = (purge_audit_log_1.entity_uuid)::text)
Rows Removed by Filter: 4500
-> GroupAggregate (actual time=0.022..0.023 rows=1 loops=1)
-> Sort (actual time=0.019..0.020 rows=5 loops=1)
-> Index Scan using ix_purge_audit_log_status_created_on on purge_audit_log purge_audit_log_2 (actual time=0.008..0.
-> Materialize (actual time=5.267..19.511 rows=5436 loops=1)
-> Nested Loop (actual time=5.266..18.722 rows=5436 loops=1)
-> Subquery Scan on preceding_groups (actual time=5.258..11.609 rows=6043 loops=1)
Rows Removed by Filter: 5
-> WindowAgg (actual time=5.256..10.858 rows=6048 loops=1)
-> Subquery Scan on blocked_timestamp_groups (actual time=5.251..7.501 rows=6048 loops=1)
-> GroupAggregate (actual time=5.250..7.116 rows=6048 loops=1)
-> Sort (actual time=5.247..5.432 rows=6048 loops=1)
-> Seq Scan on purge_audit_log purge_audit_log_3 (actual time=0.019..1.701 rows=6048 l
Rows Removed by Filter: 4757
-> Index Scan using ix_purge_audit_log_status_created_on on purge_audit_log purge_audit_log_1 (actual time=0.001..0.
-> Index Scan using purge_audit_log_pkey on purge_audit_log (actual time=0.043..0.043 rows=1 loops=936)
SubPlan 1
-> Aggregate (actual time=0.002..0.002 rows=1 loops=500)
-> Index Only Scan using ix_purge_audit_log_pruning on purge_audit_log streak_break (actual time=0.002..0.002 rows=5 loops=500
Heap Fetches: 464
SubPlan 2
-> Aggregate (actual time=0.002..0.002 rows=1 loops=464)
-> Index Only Scan using ix_purge_audit_log_pruning on purge_audit_log streak_break_1 (actual time=0.002..0.002 rows=5 loops=4
Heap Fetches: 464
-> Index Scan using ix_purge_audit_log_status_created_on on purge_audit_log unresolved_attempt (actual time=0.001..0.001 rows=0 loops=500)
Execution Time: 61.211 ms
RUN 1: discovered=500 valid=500
RUN 2: discovered=500 valid=500
RUN 3: discovered=500 valid=500
RUN 4: discovered=500 valid=500
RUN 5: discovered=500 valid=500
=== TIMING [window] PostgreSQL over 5 runs (median / max ms) ===
```
</details>
<details><summary>MySQL 8.0.44 (REPEATABLE READ) — locked re-check plan, batch 500 (EXPLAIN ANALYZE, abridged)</summary>
```
-> Remove duplicate purge_audit_log rows using temporary table (weedout) (actual time=127..139 rows=500 loops=1)
-> Filter: ((repeat_boundary.boundary_ts is null) or (preceding_groups.prev_ts > repeat_boundary.boundary_ts)) (actual time=127..139 rows=500 loop
-> Nested loop left join (actual time=127..139 rows=500 loops=1)
-> Nested loop antijoin (actual time=127..138 rows=500 loops=1)
-> Nested loop inner join (actual time=127..137 rows=500 loops=1)
-> Nested loop inner join (actual time=0.0554..9.24 rows=500 loops=1)
-> Filter: ((purge_audit_log.`status` = 'blocked') and (purge_audit_log.entity_type = 'measure_slices') and (purge_audit_log.i
? O?t??',' ?\'?M??\\ʊ!-.','`Vo?\Z(CC?-?t???','??V?TJV???q?m?','ͻ??)?O???W lÞ?','???}On?S\'?e??','o:U+\\Fk???s?\'??','+?????G??Қk??\\','3_U
?y@?^Q:??','?Y????N?E?,??0','???
?A%?Mv??5?Y','v?o?ZN???9f???Z','ܮ??M??Њ???f','Z9??b?D(??? z=?','p???? C?oAY?K','?Z)`??F??[*~??a>','???֟@??W4?g]E','??}??3EU??b\Z?','|?n???@
??v6Bb???f?L','9vog??C ? ??? ','O?(??`H ?V?qA?r','n% ?k%J??T??w%#','??Z??<I??@L????','Qt????K?????1?+','?5?پJ???B?????','(~H`?JW?:??M?D?','
[?','?_???SF_??9????T','?????yC???C??`?~','?7*z6EN?1?Usz?','as?
A????P????','s??hdD???SQ?}L','??3?xWA{??\\?;?l','?R?4qH-?\\?ra?ym','?Oa???Cc??2J??Jt','?gt7x?K??O\'?y? ?','?)"? E??٘?%','?
?wE?B̅??g+ p',' 4???JJ???l&??X','????I?B5? ?.K?? ','ŁP???@???2??(?','?`$Q??G?????9??~','?rŬ*/J5?ŕ^ȰE?','bO{? Aa?]QW?','??x??f@???? ?1R?','h???g
c','?SZ ??I%?l??I?','??q ?F&?\r?|9??/','g.J?A¡m-u\0A','???z/?D??8^???f','#?.???LX?t{?#?r','Õ?m?,Gn???m?I??','?3?HK?ME?z)??ɚ?','.???X?Fn??Q?]?
???q??','ז?(iEr??|?????','??>|s E~??c?D???','??"?8BF?a?A
?','?+??{?Je???3ɹ','??Y8D?F"?y(????d','5??0?Lu?fbr?Ԧ','?e???XN?c?i?','?\Z;:?dM?"??v)?','X\'???_H???#??? ','? x??NOr?? T\'??','?t???E??-
','?-Y.?J֍?>?!?Ո','?Q; ]E??j̘??y','?;g??S@????/{?g?','??8?? O{???>M?/','$x??E???
f??','?C?B?N(?:??]??','ߧi~?@̬??=??/?','????@ ?Jx\rJS]m',' ?Vj??Fm????G??\r','iV\\X;K??F?Rؚ)f','??O B&???z?i??','d?څ?LN??&?ʉL','???Y;?H{?;?
','??͇??O??E?*?us?','?fx???O\0????_??','RSk???J?F??(?H?','?3X?H9K2??kb???5','I???@??l?;','tQ???\0H????87B?',' Q??O E??????? ?','Ӎ&???FՆ??s 6?'
f????D%?;w??w','??(=?FO???v? H?',' ??GA??>Z???','n???Jm?^?k??Qp','nS???@??????mj','??e?O"BO????5\\$?','?c??C??]{?ɤ??','.}?lN?s?; ??',
????G٭>','?I?a#G???4?6,?','P?0??*M0?!??9??A','??\r@?KIw???G?-??','??&?Hԗ?5)#?','?Ip???I??@6?~??','?OI?ĴA ????Z??','x????iG3?,BN????','cI?T&\0M
QM?A???\\???K','?(??`?A_?n??*ڧ','?q?>?J_??#\\?\0G','? ?\'?mC??/L\Z??51','u\r??f1I?W?ztH??','X???sF???J\\??p','Ph?Z[`D???Ƣ???','$ ??U-Fm?BHe/?H
RZK','K41??\'M ?B??-F??','?IL??A???[????n','?.?q?Av? ?k?b??','???s??LI???????d','????C]Gl?xB ?x~','????M?O(?17?AX ;','?M?WTVF:??h???','Ī??1Dc?f?
!D??','m?(zgAN???;??(',' OP?H?G2???ZF
?',':??ۉ?D?????B','?,㢠?H??BG???f','??? R?LR????A??s','u???M?J??&??','W = C??k??s??','??/?L??@]v??w','eR??pQM@?????ș','?.w?D?M??
???5B','??4T??Iе???8?0?','??Pu?J????Cg?I','$?\'`Gܝ??????','\0??TnA?#;Y<L','g?? MiLx??po\ru6?','??;\06$Aۿ??? 7?','??I?RLv?\\v?GO??','?)?G?\
?X?','?d?d8L??`\'X???','F\\?uaAF?V?k(2?','^??fNOi??? ?','?lר5K ?????{?z','\'?|?H?C.?hl?|)V','\0yo4\'JΉ??b???','/h?<?*Kc????u ?','n???5D??
K/d9','G?????B ??U????',',u?GxC%?C6?????','?戄??H???j?(??','t?⪳O???(?#T','?{ ???O|???x?_?E','a??mڗAބ ??k',';.ndbGZ??? *ˆ','??J ?"A??Ή???:'
C??Ý#\\???','?\'U??BH??{L??Z\\','?S}XO??g9j?? ','?oV?J????Cм?z',' ?Q)3?D???\0c???','=??\'xM+??[Ϡ?1S','vkj T?O̧?:??ъ?','???)4?@ء?|?5?<?','?)??7
??CN????? w!p','8d,?$?F???? g+!?','R?8v?FE#????/?','mcP?b<Kw?:I?-??','?5?@1\rD??Kah??N@','??뮱DMȺ?? h*?','y????M??? \Z?ᘥ','6s?1>Cv?u???0Y','?? ?
KH???y_?`K','mi?\r?F??IC??6? ','?.<?E???w8h???','5?L9d?Fy???F??$','????R?E<??pug2?`','aϢ M\'A??vΔ\r e','q?-\rk?Og???AA???','ϳ`nGӅ?? ?vo?','?
SLO??\r j?{','?, BPEP???ʦ?J','*"??SKt?k?n???','?v?c?AA? ds??"','K~z??Lm?????ґ2','??? \\vCC?]D???OD','M?T??]K??5??A ?','M0??ZE ?`????',' Ҭ?
<','O??T1IC??%???SF','?\\?B aA????U???','W?y?\0UL??VQ?p??H','.ʅ?-~Bˬ???@?V?','?*??Fd??NyK?
[','(XZ5-NO
?
?|?aF1','[!fl\Z?J&?&?? ͅ?','?,?:??D??%? U?','???>\Z@r??? K?')) and (purge_audit_log.`trigger` <> 'force') and (purge_audit_log.entity_uuid is not
-> Index range scan on purge_audit_log using PRIMARY over (id = 0x00796f1034274ace89f89d62d3d47fc3) OR (id = 0x017926c6927
-> Filter: ((purge_audit_log.`status` = 'blocked') and (purge_audit_log.entity_uuid is not null) and (purge_audit_log.created_
-> Single-row index lookup on purge_audit_log using PRIMARY (id=purge_audit_log.id) (actual time=690e-6..709e-6 rows=1 loo
-> Select #2 (subquery in condition; dependent)
-> Aggregate: max(streak_break.created_on) (actual time=0.00719..0.00721 rows=1 loops=500)
-> Filter: ((streak_break.entity_type = purge_audit_log.entity_type) and (streak_break.entity_uuid = purge_audit_l
-> Covering index range scan on streak_break using ix_purge_audit_log_pruning over (status = 'confirmed') OR (
-> Select #3 (subquery in condition; dependent)
-> Aggregate: max(streak_break.created_on) (actual time=0.00729..0.00731 rows=1 loops=464)
-> Filter: ((streak_break.entity_type = purge_audit_log.entity_type) and (streak_break.entity_uuid = purge_audit_l
-> Covering index range scan on streak_break using ix_purge_audit_log_pruning over (status = 'confirmed') OR (
-> Filter: ((preceding_groups.entity_type = 'measure_slices') and (preceding_groups.prev_min_reason <=> purge_audit_log.reason) an
-> Index lookup on preceding_groups using <auto_key0> (entity_uuid=purge_audit_log.entity_uuid, ts=purge_audit_log.created_on,
-> Materialize (actual time=127..127 rows=6048 loops=1)
-> Window aggregate with buffering: lag(max_reason) OVER (PARTITION BY blocked_timestamp_groups.entity_type,blocked_ti
-> Table scan on <temporary> (actual time=99.3..99.9 rows=6048 loops=1)
-> Temporary table (actual time=99.3..99.3 rows=6048 loops=1)
-> Window aggregate with buffering: lag(min_reason) OVER (PARTITION BY blocked_timestamp_groups.entity_typ
-> Table scan on <temporary> (actual time=78.7..79.4 rows=6048 loops=1)
-> Temporary table (actual time=78.7..78.7 rows=6048 loops=1)
-> Window aggregate with buffering: lag(n_coded) OVER (PARTITION BY blocked_timestamp_groups.e
-> Table scan on <temporary> (actual time=56.6..57.3 rows=6048 loops=1)
-> Temporary table (actual time=56.6..56.6 rows=6048 loops=1)
-> Window aggregate with buffering: lag(n) OVER (PARTITION BY blocked_timestamp_gr
-> Table scan on <temporary> (actual time=36.4..37.1 rows=6048 loops=1)
-> Temporary table (actual time=36.4..36.4 rows=6048 loops=1)
-> Window aggregate with buffering: lag(purge_audit_log.ts) OVER (PART
-> Sort: blocked_timestamp_groups.entity_type, blocked_timestamp_g
-> Table scan on blocked_timestamp_groups (actual time=15.6..1
-> Materialize (actual time=15.6..15.6 rows=6048 loops=1)
-> Group aggregate: count(0), count(purge_audit_log.re
-> Filter: ((purge_audit_log.entity_type = 'measur
-> Index scan on purge_audit_log using ix_purg
-> Filter: ((unresolved_attempt.entity_uuid = purge_audit_log.entity_uuid) and (unresolved_attempt.entity_type = purge_audit_log.entit
-> Index lookup on unresolved_attempt using ix_purge_audit_log_status_created_on (status='pending') (actual time=0.00108..0.00108
-> Filter: ((repeat_boundary.entity_type = purge_audit_log.entity_type) and (repeat_boundary.entity_uuid = purge_audit_log.entity_uuid)) (
-> Covering index lookup on repeat_boundary using <auto_key0> (entity_type=purge_audit_log.entity_type, entity_uuid=purge_audit_log.en
-> Materialize (actual time=0.068..0.068 rows=1 loops=1)
-> Table scan on <temporary> (actual time=0.0455..0.0455 rows=1 loops=1)
-> Aggregate using temporary table (actual time=0.0446..0.0446 rows=1 loops=1)
-> Filter: ((purge_audit_log.entity_type = 'measure_slices') and (purge_audit_log.`status` in ('confirmed','target_abs
-> Covering index range scan on purge_audit_log using ix_purge_audit_log_pruning over (status = 'confirmed' AND en
RUN 1: discovered=500 valid=500
RUN 2: discovered=500 valid=500
RUN 3: discovered=500 valid=500
RUN 4: discovered=500 valid=500
RUN 5: discovered=500 valid=500
```
</details>
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend query shape; numbers above).
### TESTING INSTRUCTIONS
```
pytest tests/unit_tests/commands/deletion_retention/test_prune_audit.py # 66, incl. lock/scope order pin, bind ceiling on SQLite, lag()-without-CTE guard
pytest tests/integration_tests/deletion_retention/prune_audit_tests.py # 35, incl. the 40-seed legacy-vs-new equivalence test (run on PG and MySQL)
```
To reproduce the numbers, run the harness in the evidence directory named above in mode `window` against a PostgreSQL and a MySQL 8 database (`measure_lock_hold.py <uri> window 5 500`).
### ADDITIONAL INFORMATION
- [x] Has associated issue: sc-120493 (follow-up sc-120950)
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5767661483
Comment follow-up at `1e2982ea941e126924cb0a5fc9418e4259a69b65`: added direct replies on the three remaining fixed inline findings (scalar boundary entity type, evidence ID-only projection, and scalar boundary UUID isolation). The SQLite version-floor fix is covered in the earlier final-fixes summary: https://github.com/apache/superset/pull/44262#issuecomment-5765672563 . Current-head CI has now completed without failing or pending checks.
For completeness, acknowledging Bito's older duplicated-test-setup suggestion: https://github.com/apache/superset/pull/44262#issuecomment-5720512336 . The tests still use separate setup; that cleanup has not been implemented and is not being claimed fixed. The earlier boundary-logic duplication suggestion was already recorded as a structural follow-up: https://github.com/apache/superset/pull/44262#issuecomment-5708834664 .
Thanks @aminghadersohi for the latest execution check confirming the UUID gap is closed and the production-change mutants are caught. Human threads are left for their reviewers to resolve; final approval is still outstanding.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4064306671
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -615,15 +800,17 @@ def _evidence_candidates(now: datetime, cutoff: datetime, limit: int) -> sa.sql.
"""
table: sa.Table = PurgeAuditLog.__table__
return (
- sa.select(table.c.id)
+ sa.select(table.c.id, table.c.entity_type, table.c.entity_uuid)
Review Comment:
`_EvidenceRecheck.__call__` ignores scope entirely, so `entity_type`/`entity_uuid` here get fetched per candidate row for nothing.
```suggestion
sa.select(table.c.id)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4033215284
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -372,53 +457,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: the sc-120493 Variant 2
+ measurements 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
+ unlocked discovery. 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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag the independent entity-type/UUID IN-list cross-product in the prune-audit scope helper; it is an intentional bounded fallback tradeoff with candidate gates and differential tests.
**Applied to:**
- `superset/commands/deletion_retention/prune_audit.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -485,32 +665,33 @@ def _duplicate_candidates(now: datetime, limit: int) -> sa.sql.Select:
"""
table: sa.Table = PurgeAuditLog.__table__
return (
- sa.select(table.c.id)
+ sa.select(table.c.id, table.c.entity_type, table.c.entity_uuid)
.where(*_duplicate_predicates(table, now))
.order_by(table.c.created_on)
.limit(limit)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require LIMIT to bound discovery CPU or age-unbounded history scans in the retention-pruning implementation; treat the disclosed limitation as intentional unless measured optimization is explicitly in scope.
**Applied to:**
- `superset/commands/deletion_retention/prune_audit.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4033213656
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -372,53 +457,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: the sc-120493 Variant 2
+ measurements 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
+ unlocked discovery. 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:
Good catch on the extra histories: this is a disclosed performance tradeoff, not exact-pair scoping. At `db03401048de06282835bab5e86bf37db2f407ff`, the scope helper documents the type/UUID cross-product; final locked candidacy is still restricted to discovered IDs, and the shared-UUID/different-type regression remains. Could we keep the ratified default50/max100 ceiling and track plan improvements with Amin-owned #44349 rather than introducing another predicate rewrite here? #44349 is downstream of this PR and needs to retain the fallback, candidate gates and differential tests. No bounded lock-time or production-capacity guarantee is claimed.
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -485,32 +665,33 @@ def _duplicate_candidates(now: datetime, limit: int) -> sa.sql.Select:
"""
table: sa.Table = PurgeAuditLog.__table__
return (
- sa.select(table.c.id)
+ sa.select(table.c.id, table.c.entity_type, table.c.entity_uuid)
.where(*_duplicate_predicates(table, now))
.order_by(table.c.created_on)
.limit(limit)
Review Comment:
Agreed that LIMIT does not bound discovery's history work. The published code explicitly describes the age-unbounded unlocked scan, and the title/operator guidance no longer claim O(batch) or a short indexed window. Could we retain this disclosed limitation for this scoped fix and coordinate measured optimization with downstream #44349? Default50/max100 bounds the candidate batch and placeholder budget, not discovery CPU or the scoped history scanned during recheck. Live MySQL5.7/MariaDB validation and production capacity remain unclaimed; no production sizing assumptions are inferred from the synthetic measurements.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4039499608
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,621 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
+@pytest.mark.parametrize(
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect: Any) -> None:
+ """Limit history to candidate entities only when a batch scope is supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now, scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
+@pytest.mark.parametrize(
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
+@pytest.mark.parametrize("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
+@pytest.mark.parametrize(
+ "scope_entities",
+ [None, [("chart", "shared"), ("dashboard", "shared")], []],
+)
+def test_repeat_paths_isolate_entity_types_and_agree(
+ scope_entities: list[tuple[str, str | None]] | None,
+) -> None:
+ """Keep each type's first block when UUIDs collide, in both query paths."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(3)]
+ rows: list[dict[str, Any]] = [
+ {
+ "id": id_,
+ "entity_type": entity_type,
+ "entity_uuid": "shared",
+ "status": STATUS_BLOCKED,
+ "trigger": "scheduled",
+ "actor": "system",
+ "created_on": datetime(2026, 1, day),
+ "reason": "same",
Review Comment:
Good catch; addressed in e16fe624684ba50877b2570c658455896b170819 by varying the reason by entity type as suggested. Removing the between.entity_type conjunct passes the old fixture but fails the amended fixture; restoration passes, independently reproduced in review. All 105 affected unit tests and mandatory branch-file hooks including MyPy passed; fresh CI is queued. Could you take another look? The adjacent earlier.entity_type discrimination case remains an explicitly recorded follow-up, not coverage claimed by this change.
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,621 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
+@pytest.mark.parametrize(
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect: Any) -> None:
+ """Limit history to candidate entities only when a batch scope is supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now, scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
+@pytest.mark.parametrize(
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
+@pytest.mark.parametrize("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
+@pytest.mark.parametrize(
+ "scope_entities",
+ [None, [("chart", "shared"), ("dashboard", "shared")], []],
+)
+def test_repeat_paths_isolate_entity_types_and_agree(
+ scope_entities: list[tuple[str, str | None]] | None,
+) -> None:
+ """Keep each type's first block when UUIDs collide, in both query paths."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(3)]
+ rows: list[dict[str, Any]] = [
+ {
+ "id": id_,
+ "entity_type": entity_type,
+ "entity_uuid": "shared",
+ "status": STATUS_BLOCKED,
+ "trigger": "scheduled",
+ "actor": "system",
+ "created_on": datetime(2026, 1, day),
+ "reason": "same",
+ }
+ for id_, entity_type, day in zip(
+ ids, ["chart", "dashboard", "chart"], [1, 2, 3], strict=True
+ )
+ ]
+ try:
+ metadata.create_all(engine)
+ with engine.begin() as connection:
+ connection.execute(sa.insert(table), rows)
+ predicate: Callable[..., sa.ColumnElement[bool]]
+ for predicate in (
+ prune_audit._window_repeats_an_earlier_block,
+ prune_audit._legacy_repeats_an_earlier_block,
+ ):
+ assert set(
+ connection.scalars(
+ sa.select(table.c.id).where(
+ table.c.id.in_(ids if scope_entities != [] else []),
+ predicate(table, datetime(2026, 2, 1)),
Review Comment:
Good catch; addressed in e16fe624684ba50877b2570c658455896b170819. The misleading parameterization is removed, and a separate fixture passes scope_entities directly into the window predicate with no outer ID filter: unscoped, two-pair, one-pair and empty scope yield 3/2/1/0 repeats. Forcing scope to None produces three failures; restoration passes, independently reproduced in review. Could you check that this captures the intended contract? The legacy path intentionally has no scope parameter; deliberate cross-product widening remains documented rather than newly pinned by this fixture.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5707233373
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5705688236
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 6d41ae49f4871305726ffc42f284d2b126df58ea |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aab1d3a86f71400080156af |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44262--superset-docs-preview.netlify.app](https://deploy-preview-44262--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgEADQu8w6Yexjnxt0jNHRkkrFgojo7rl7i_dCyvFIF0wAFehMwazFpxGyxnZsRMrWWPZhEXqPZhO3DxzvctmmPT4zl62ItOSGBuV62SLigLJT5OD7AQ) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44262--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5765672563
@aminghadersohi @rusackas the remaining review fixes are published in `1e2982ea941e126924cb0a5fc9418e4259a69b65`:
- SQLite now initializes its dialect version before choosing the repeat-query path; versions below 3.25 and still-unknown versions use the legacy predicate.
- Evidence discovery selects only IDs, preserving the locked recheck/delete behavior.
- The isolation histories now cover both entity type and UUID. Removing the boundary UUID guard passed the prior 157-test candidate suite; the new two-chart/different-UUID regression fails under that mutation and passes when restored. The earlier cross-type fixture is preserved.
Fresh local validation: 161 deletion-retention tests and all applicable full-branch-file hooks pass. Independent Astra final review approved the exact published tree, including its own SQL/mutation control. Old-SQLite/MySQL/MariaDB runtime and capacity measurements remain disclosed limits; mocked version boundaries aren't live old-engine evidence.
New-head CI is running. Could you take a final pass on these fixes and re-approve if satisfied? No retention defaults or feature enablement changed. Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5745806577
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): compute the purge-audit repeat predicate once per entity set — lock-hold O(batch) on every backend [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5670882580
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44262?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `10.52632%` with `17 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.39%. Comparing base ([`43a0031`](https://app.codecov.io/gh/apache/superset/commit/43a0031995636537ce5ff8909e4b7de07a0ee4d5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0cf3835`](https://app.codecov.io/gh/apache/superset/commit/0cf3835ac0f3164b4236b1d22e2fab381ff2552b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44262?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...uperset/commands/deletion\_retention/prune\_audit.py](https://app.codecov.io/gh/apache/superset/pull/44262?src=pr&el=tree&filepath=superset%2Fcommands%2Fdeletion_retention%2Fprune_audit.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGVsZXRpb25fcmV0ZW50aW9uL3BydW5lX2F1ZGl0LnB5) | 10.52% | [17 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44262 +/- ##
==========================================
- Coverage 57.42% 57.39% -0.04%
==========================================
Files 2926 2926
Lines 172919 172931 +12
Branches 40115 40116 +1
==========================================
- Hits 99291 99246 -45
- Misses 72685 72742 +57
Partials 943 943
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44262/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44262/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44262/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.21% <10.52%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44262/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.21% <10.52%> (-0.06%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44262?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): compute the purge-audit repeat predicate once per entity set — lock-hold O(batch) on every backend [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5670825861
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5744697782
@richardfogaca Good catch—added the boundary-join counterexample and a same-reason cross-type predecessor case in https://github.com/apache/superset/commit/b472597d95, retaining the original partition case. All three entity_type mutations are caught: removing the LAG partition guard produces 2 failed cases; removing the boundary-join guard or legacy earlier.entity_type guard each produces 1 failed case. Both unmodified paths pass all three cases; the affected unit suites pass 151 tests and branch-file hooks pass. Also documented that dispatch trusts SELECT VERSION(), including the synthetic-8.x proxy limitation. No production code changed; live MySQL 5.7/MariaDB validation remains unexercised here.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): compute the purge-audit repeat predicate once per entity set — lock-hold O(batch) on every backend [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4013968037
##########
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:
This line is accurate, but the PR title's "lock-hold O(batch) on every backend" contradicts it. Holding batch at 50 and raising history depth 200->12800 (64x), the locked re-check went 1.97ms -> 109ms (55x): the group and LAG tables scan each scoped entity's full history, unfiltered by batch id.
##########
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:
Load-bearing but unpinned: dropping `entity_type` from this partition leaves all 66 unit + 34 integration tests green, while making a row take its predecessor from a different entity_type sharing the uuid and be deleted as a repeat. The equivalence test seeds a single entity_type.
##########
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:
`lag()` is the only window function in the metadata-DB query surface (master has none), yet configuring-superset.mdx:287 still lists MySQL 5.7, which has no window functions. CI only covers mysql:8.0, and pruning runs by default, so a 5.7 metadata DB would fail at runtime.
##########
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:
`scope_entities` is never read in this body: compiled at MAX_BATCH_SIZE the evidence re-check binds 507 placeholders vs 3,520 for duplicate. `_delete_batch` still runs the scope lookup under the coordination lock for this category, so it is a locked round-trip with no consumer.
##########
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:
`lock-hold-evidence/REPORT.md` does not exist in this repo — the PR body places it in a separate spec repo. A shipped docstring should not cite a path no maintainer can open.
```suggestion
Keep the repeat-id query uncorrelated: the sc-120493 Variant 2
measurements showed MySQL repeatedly executing
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): compute the purge-audit repeat predicate once per entity set — lock-hold O(batch) on every backend [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5708834664
Prepared by Codex at Mike's request. Feedback accounting at `db03401048de06282835bab5e86bf37db2f407ff`: original five inline findings have individual remediation/evidence replies (starting https://github.com/apache/superset/pull/44262#discussion_r4031489900). The two newer cross-product/discovery-cost comments are acknowledged as retained limitations, not claimed fixed: https://github.com/apache/superset/pull/44262#discussion_r4033213656 and https://github.com/apache/superset/pull/44262#discussion_r4033213782 . Bito's original fallback and unused-scope points map to those existing fixes; its boundary-logic duplication suggestion is left as a structural follow-up, with semantics pinned by differential tests rather than another rewrite in this packet.
Repaired-master PostgreSQL CI completes `2791 passed, 165 skipped`: https://github.com/apache/superset/actions/runs/35166198065/job/105027831036 . This supersedes the earlier multiple-head setup failure. The ratified default50/max100, legacy/unknown-version fallback and no O(batch) guarantee remain. Prior audit follow-ups (backlog metric ambiguity, read-only transaction lifetime, scope-test fence and dialect dispatch evidence) are not silently marked fixed. #44349 remains Amin-owned and downstream; no MySQL5.7/MariaDB live execution or production-capacity validation is claimed. Reviewer acceptance remains outstanding.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5766992372
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #44262:
URL: https://github.com/apache/superset/pull/44262
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4055041837
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,701 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
+@pytest.mark.parametrize(
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect: Any) -> None:
+ """Limit history to candidate entities only when a batch scope is supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now, scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
+@pytest.mark.parametrize(
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
+@pytest.mark.parametrize("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
+@pytest.mark.parametrize(
+ ("history", "repeat_indices"),
+ [
+ pytest.param(
+ [
+ ("chart", STATUS_BLOCKED, 1, "same"),
+ ("dashboard", STATUS_BLOCKED, 2, "other"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="partition-reason-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_CONFIRMED, 1, None),
+ ("chart", STATUS_BLOCKED, 5, "same"),
+ ("chart", STATUS_CONFIRMED, 6, None),
+ ("chart", STATUS_BLOCKED, 7, "same"),
+ ("chart", STATUS_BLOCKED, 8, "same"),
+ ],
+ [4],
+ id="boundary-join-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_BLOCKED, 1, "same"),
+ ("chart", STATUS_BLOCKED, 2, "same"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="predecessor-type-isolation",
+ ),
Review Comment:
Production is correct, but `_streak_boundary_scalar`'s `entity_type` conjunct (`prune_audit.py:325`) survives all 151 unit tests and the 40-seed equivalence test; dropping it ages the streak's earliest "blocked since" row out via the operational category. This case reds it on the legacy path:
```suggestion
),
pytest.param(
[
("chart", STATUS_BLOCKED, 5, "same"),
("chart", STATUS_BLOCKED, 7, "same"),
("dashboard", STATUS_CONFIRMED, 9, None),
],
[1],
id="foreign-newer-streak-breaker",
),
```
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,621 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
+@pytest.mark.parametrize(
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect: Any) -> None:
+ """Limit history to candidate entities only when a batch scope is supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now, scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
+@pytest.mark.parametrize(
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
+@pytest.mark.parametrize("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
+@pytest.mark.parametrize(
+ "scope_entities",
+ [None, [("chart", "shared"), ("dashboard", "shared")], []],
+)
+def test_repeat_paths_isolate_entity_types_and_agree(
+ scope_entities: list[tuple[str, str | None]] | None,
+) -> None:
+ """Keep each type's first block when UUIDs collide, in both query paths."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(3)]
+ rows: list[dict[str, Any]] = [
+ {
+ "id": id_,
+ "entity_type": entity_type,
+ "entity_uuid": "shared",
+ "status": STATUS_BLOCKED,
+ "trigger": "scheduled",
+ "actor": "system",
+ "created_on": datetime(2026, 1, day),
+ "reason": "same",
Review Comment:
Confirmed at `b472597d`: dropping `between.entity_type` reds `partition-reason-isolation`, restoring it passes, and the 40-seed equivalence test is green both ways. The `earlier.entity_type` follow-up is covered too — `predecessor-type-isolation` kills it.
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,621 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
+@pytest.mark.parametrize(
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect: Any) -> None:
+ """Limit history to candidate entities only when a batch scope is supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now, scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
+@pytest.mark.parametrize(
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
+@pytest.mark.parametrize("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
+@pytest.mark.parametrize(
+ "scope_entities",
+ [None, [("chart", "shared"), ("dashboard", "shared")], []],
+)
+def test_repeat_paths_isolate_entity_types_and_agree(
+ scope_entities: list[tuple[str, str | None]] | None,
+) -> None:
+ """Keep each type's first block when UUIDs collide, in both query paths."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(3)]
+ rows: list[dict[str, Any]] = [
+ {
+ "id": id_,
+ "entity_type": entity_type,
+ "entity_uuid": "shared",
+ "status": STATUS_BLOCKED,
+ "trigger": "scheduled",
+ "actor": "system",
+ "created_on": datetime(2026, 1, day),
+ "reason": "same",
+ }
+ for id_, entity_type, day in zip(
+ ids, ["chart", "dashboard", "chart"], [1, 2, 3], strict=True
+ )
+ ]
+ try:
+ metadata.create_all(engine)
+ with engine.begin() as connection:
+ connection.execute(sa.insert(table), rows)
+ predicate: Callable[..., sa.ColumnElement[bool]]
+ for predicate in (
+ prune_audit._window_repeats_an_earlier_block,
+ prune_audit._legacy_repeats_an_earlier_block,
+ ):
+ assert set(
+ connection.scalars(
+ sa.select(table.c.id).where(
+ table.c.id.in_(ids if scope_entities != [] else []),
+ predicate(table, datetime(2026, 2, 1)),
Review Comment:
Confirmed. `test_window_repeat_predicate_applies_entity_scope` passes `scope_entities` straight into the window predicate and the four arms give 3/2/1/0 repeats; that is the contract. It is not scope-only either — dropping the LAG partition reds two of its arms.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5705786085
CI update for 6d41ae49f4871305726ffc42f284d2b126df58ea: the current Docker setup job fails with multiple Alembic heads before tests: https://github.com/apache/superset/actions/runs/35159600323/job/105007164634 . Independent graph inspection of apache master at 7b6dd7597c4af49f7f0571b7b33dd247f382218b finds heads 0884f655c0a6 and e2f3a1b9c640; this PR changes no migrations. The local 147-unit/35-PostgreSQL integration results do not make CI green. Upstream reconciliation and current-head CI remain required.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4031974732
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -372,53 +457,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: the sc-120493 Variant 2
+ measurements 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
+ unlocked discovery. 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:
**Suggestion:** Independent `IN` lists create a cross-product of entity types and UUIDs, causing locked rechecks to process histories for entity pairs that were never discovered. [performance]
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d6be76148c014349ba32f4ff59c6eb26&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d6be76148c014349ba32f4ff59c6eb26&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/deletion_retention/prune_audit.py
**Line:** 478:480
**Comment:**
*Performance: Independent `IN` lists create a cross-product of entity types and UUIDs, causing locked rechecks to process histories for entity pairs that were never discovered.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44262&comment_hash=94d039b5b6edaf3c65117c177db7fd2479761f45538cded0b094c5bc2c276fdb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44262&comment_hash=94d039b5b6edaf3c65117c177db7fd2479761f45538cded0b094c5bc2c276fdb&reaction=dislike'>👎</a>
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -485,32 +665,33 @@ def _duplicate_candidates(now: datetime, limit: int) -> sa.sql.Select:
"""
table: sa.Table = PurgeAuditLog.__table__
return (
- sa.select(table.c.id)
+ sa.select(table.c.id, table.c.entity_type, table.c.entity_uuid)
.where(*_duplicate_predicates(table, now))
.order_by(table.c.created_on)
.limit(limit)
Review Comment:
**Suggestion:** Discovery builds the full blocked-row, timestamp-group, and boundary histories before applying `LIMIT`, so every batch rescans and groups the entire audit table on window-capable databases. [performance]
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=38c7f8c7850a4fafba5e2cd4c7d87713&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=38c7f8c7850a4fafba5e2cd4c7d87713&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/deletion_retention/prune_audit.py
**Line:** 668:671
**Comment:**
*Performance: Discovery builds the full blocked-row, timestamp-group, and boundary histories before applying `LIMIT`, so every batch rescans and groups the entire audit table on window-capable databases.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44262&comment_hash=968cd39a6d204132d6f1e0d036402e97daf9d96f7cce2cf33f2880382b9f6b03&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44262&comment_hash=968cd39a6d204132d6f1e0d036402e97daf9d96f7cce2cf33f2880382b9f6b03&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4064893029
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,710 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
+@pytest.mark.parametrize(
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect: Any) -> None:
+ """Limit history to candidate entities only when a batch scope is supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now, scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
+@pytest.mark.parametrize(
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
+@pytest.mark.parametrize("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
+@pytest.mark.parametrize(
+ ("history", "repeat_indices"),
+ [
+ pytest.param(
+ [
+ ("chart", STATUS_BLOCKED, 1, "same"),
+ ("dashboard", STATUS_BLOCKED, 2, "other"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="partition-reason-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_CONFIRMED, 1, None),
+ ("chart", STATUS_BLOCKED, 5, "same"),
+ ("chart", STATUS_CONFIRMED, 6, None),
+ ("chart", STATUS_BLOCKED, 7, "same"),
+ ("chart", STATUS_BLOCKED, 8, "same"),
+ ],
+ [4],
+ id="boundary-join-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_BLOCKED, 1, "same"),
+ ("chart", STATUS_BLOCKED, 2, "same"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="predecessor-type-isolation",
+ ),
+ pytest.param(
+ [
+ ("chart", STATUS_BLOCKED, 5, "same"),
+ ("chart", STATUS_BLOCKED, 7, "same"),
+ ("dashboard", STATUS_CONFIRMED, 9, None),
+ ],
+ [1],
+ id="foreign-newer-streak-breaker",
Review Comment:
Same function, other half (`prune_audit.py:326`): dropping `boundary.entity_uuid == target.entity_uuid` survives all 152 unit tests and the 40-seed equivalence test — two charts, different UUIDs, one confirmed newer, and the other's "blocked since" row ages out. Fixture hardcodes the UUID, no fence.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4066510538
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,701 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
+@pytest.mark.parametrize(
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect: Any) -> None:
+ """Limit history to candidate entities only when a batch scope is supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now, scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
+@pytest.mark.parametrize(
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
+@pytest.mark.parametrize("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
+@pytest.mark.parametrize(
+ ("history", "repeat_indices"),
+ [
+ pytest.param(
+ [
+ ("chart", STATUS_BLOCKED, 1, "same"),
+ ("dashboard", STATUS_BLOCKED, 2, "other"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="partition-reason-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_CONFIRMED, 1, None),
+ ("chart", STATUS_BLOCKED, 5, "same"),
+ ("chart", STATUS_CONFIRMED, 6, None),
+ ("chart", STATUS_BLOCKED, 7, "same"),
+ ("chart", STATUS_BLOCKED, 8, "same"),
+ ],
+ [4],
+ id="boundary-join-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_BLOCKED, 1, "same"),
+ ("chart", STATUS_BLOCKED, 2, "same"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="predecessor-type-isolation",
+ ),
Review Comment:
Addressed in `1e2982ea941e126924cb0a5fc9418e4259a69b65`: the foreign-newer-streak-breaker case now pins the scalar boundary's entity_type guard, and the existing isolation cases are retained. Thanks for confirming the mutation is caught in your subsequent review.
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -615,15 +800,17 @@ def _evidence_candidates(now: datetime, cutoff: datetime, limit: int) -> sa.sql.
"""
table: sa.Table = PurgeAuditLog.__table__
return (
- sa.select(table.c.id)
+ sa.select(table.c.id, table.c.entity_type, table.c.entity_uuid)
Review Comment:
Addressed in `1e2982ea941e126924cb0a5fc9418e4259a69b65`: evidence discovery now selects only the audit ID, as suggested. The locked candidacy recheck and delete behavior are preserved. The final affected suite passed all 161 tests.
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,710 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
+@pytest.mark.parametrize(
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect: Any) -> None:
+ """Limit history to candidate entities only when a batch scope is supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now, scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
+@pytest.mark.parametrize(
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
+@pytest.mark.parametrize("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
+@pytest.mark.parametrize(
+ ("history", "repeat_indices"),
+ [
+ pytest.param(
+ [
+ ("chart", STATUS_BLOCKED, 1, "same"),
+ ("dashboard", STATUS_BLOCKED, 2, "other"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="partition-reason-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_CONFIRMED, 1, None),
+ ("chart", STATUS_BLOCKED, 5, "same"),
+ ("chart", STATUS_CONFIRMED, 6, None),
+ ("chart", STATUS_BLOCKED, 7, "same"),
+ ("chart", STATUS_BLOCKED, 8, "same"),
+ ],
+ [4],
+ id="boundary-join-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_BLOCKED, 1, "same"),
+ ("chart", STATUS_BLOCKED, 2, "same"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="predecessor-type-isolation",
+ ),
+ pytest.param(
+ [
+ ("chart", STATUS_BLOCKED, 5, "same"),
+ ("chart", STATUS_BLOCKED, 7, "same"),
+ ("dashboard", STATUS_CONFIRMED, 9, None),
+ ],
+ [1],
+ id="foreign-newer-streak-breaker",
Review Comment:
Addressed in `1e2982ea941e126924cb0a5fc9418e4259a69b65`: added the two-chart/different-UUID isolation case. Removing the boundary entity_uuid guard fails the new case; the unmodified control passes. Thanks for independently confirming that this arm closes the gap in your latest review.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4035028005
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,621 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
+@pytest.mark.parametrize(
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect: Any) -> None:
+ """Limit history to candidate entities only when a batch scope is supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now, scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
+@pytest.mark.parametrize(
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
+@pytest.mark.parametrize("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
+@pytest.mark.parametrize(
+ "scope_entities",
+ [None, [("chart", "shared"), ("dashboard", "shared")], []],
+)
+def test_repeat_paths_isolate_entity_types_and_agree(
+ scope_entities: list[tuple[str, str | None]] | None,
+) -> None:
+ """Keep each type's first block when UUIDs collide, in both query paths."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(3)]
+ rows: list[dict[str, Any]] = [
+ {
+ "id": id_,
+ "entity_type": entity_type,
+ "entity_uuid": "shared",
+ "status": STATUS_BLOCKED,
+ "trigger": "scheduled",
+ "actor": "system",
+ "created_on": datetime(2026, 1, day),
+ "reason": "same",
+ }
+ for id_, entity_type, day in zip(
+ ids, ["chart", "dashboard", "chart"], [1, 2, 3], strict=True
+ )
+ ]
+ try:
+ metadata.create_all(engine)
+ with engine.begin() as connection:
+ connection.execute(sa.insert(table), rows)
+ predicate: Callable[..., sa.ColumnElement[bool]]
+ for predicate in (
+ prune_audit._window_repeats_an_earlier_block,
+ prune_audit._legacy_repeats_an_earlier_block,
+ ):
+ assert set(
+ connection.scalars(
+ sa.select(table.c.id).where(
+ table.c.id.in_(ids if scope_entities != [] else []),
+ predicate(table, datetime(2026, 2, 1)),
Review Comment:
`scope_entities` is never passed to `predicate(...)`, so `None` and the two-pair case run identically and `[]` only asserts an empty id list. The parameter reads as scoped/unscoped coverage it does not provide here.
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,621 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
+@pytest.mark.parametrize(
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect: Any) -> None:
+ """Limit history to candidate entities only when a batch scope is supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now, scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
+@pytest.mark.parametrize(
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
+@pytest.mark.parametrize("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
+@pytest.mark.parametrize(
+ "scope_entities",
+ [None, [("chart", "shared"), ("dashboard", "shared")], []],
+)
+def test_repeat_paths_isolate_entity_types_and_agree(
+ scope_entities: list[tuple[str, str | None]] | None,
+) -> None:
+ """Keep each type's first block when UUIDs collide, in both query paths."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(3)]
+ rows: list[dict[str, Any]] = [
+ {
+ "id": id_,
+ "entity_type": entity_type,
+ "entity_uuid": "shared",
+ "status": STATUS_BLOCKED,
+ "trigger": "scheduled",
+ "actor": "system",
+ "created_on": datetime(2026, 1, day),
+ "reason": "same",
Review Comment:
Follow-on to the LAG-partition thread: the legacy path's twin conjunct `between.entity_type == table.entity_type` can be dropped with all 147 unit tests and the 40-seed equivalence test green — every row here shares one reason, so it is trivially satisfied. Varying reason by type kills it.
```suggestion
"reason": "same" if entity_type == "chart" else "other",
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44262:
URL: https://github.com/apache/superset/pull/44262#issuecomment-5720512336
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org