mikebridge commented on PR #43490: URL: https://github.com/apache/superset/pull/43490#issuecomment-5500242020
Heads-up for re-review: I ran a focused data-systems/python/sqlalchemy review over the last delta (the reason-transition dedup + tie policy, `79ebceba18..86e86b4078`) and it surfaced one real edge case, now fixed in `a24a1313fe`: - **Differing-reason timestamp tie could delete a reason-transition run-head.** `_repeats_an_earlier_block` used strict `>`/`<` bounds, so a differing-reason block sharing an exact `created_on` with a run's earlier endpoint wasn't counted as a boundary — with `A(X)@t0`, a tied `A(X)@t1`/`B(Y)@t1`, and a later `A(X)@t2`, the last A (the first block after the reason returned to X) was pruned as a duplicate, dropping a reason-transition record. Fixed by making the boundary bounds inclusive (`>=`/`<=`), matching the preserving-side tie rule the pending and evidence guards already use; inclusive bounds can only ever preserve more, never delete more. New `test_a_tied_reason_change_still_breaks_the_run` fails on the old bounds and passes on the fix. Also in the same amend: corrected a docstring that overclaimed writer-parity for reason-less (pre-feature) runs (the pruner is stricter there — it collapses legacy duplicates; the writer never suppresses a reason-less block), and added an A,A,B,A,A reason-return test. Integration suite 28/28 on Postgres + SQLite, unit 83/83, pre-commit clean. Sorry to bounce the approval — flagging so it gets a fresh look at the new head. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
