gabotorresruiz commented on PR #44089: URL: https://github.com/apache/superset/pull/44089#issuecomment-5688569969
Post-merge follow-up, Mike. Before this merged I verified the fix end to end on a real DB: detaching a chart emits exactly one `remove` change record at the detach transaction, an unrelated title edit emits no membership records, a re-attach emits one `add`, and the same detach on the parent commit emits zero records, confirming the bug. Window semantics (half-open `[attach, detach)`, transaction-id space) and the chunked `IN` all check out, so no concerns about what landed. Two small follow-ups worth doing: - The internal tracker references are now on master: the id in the `shadow_queries.py` docstring and five spots in `test_shadow_queries.py`, including internal severity labels like `HIGH regression guard`. `membership.py` carries the same pattern from #44010. They carry no meaning for the community, so a small cleanup sweep would be good. - Nothing in CI reproduces the actual bug against real Continuum shadows: the new unit tests fail pre-fix only via `TypeError` on the changed signature, and `change_records_tests.py` has no test that changes slice membership. I wrote the missing integration case locally (attach, detach, title edit, re-attach against the birth names dashboard) and confirmed it fails semantically on the parent and passes on this change; happy to hand it over or open a small PR adding it to `TestDashboardChangeRecords`. -- 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]
