mikebridge commented on PR #41076: URL: https://github.com/apache/superset/pull/41076#issuecomment-4937282803
🤖 This review was generated via Codex using parallel Preset, Python, React, and Superset committer review passes. Thanks for the substantial work on the cross-entity activity view. The implementation and test coverage are extensive, but I found two correctness issues that should be resolved before merging. ### Blocking findings **1. The per-kind cap can omit newer activity across ID chunks** In `superset/versioning/activity/queries.py:445`, each ID chunk is queried using only the budget left by earlier chunks. Because the IDs originate from sets, chunk order is not a meaningful global ordering. An earlier chunk can consume the 5,000-row cap even when a later chunk contains newer activity. The resulting stream can therefore return older records while silently omitting newer ones. Could we instead fetch a bounded candidate set from every chunk, merge the candidates using the complete ordering key, and retain the newest global `limit` rows? Fetching or probing one additional row would also let us set `truncated` only when more data actually exists. A regression test with more than 500 related entities, where a later chunk contains the newest record, would help lock this down. **2. Related history is not bounded to the current entity incarnation** In `superset/versioning/activity/scope.py:73`, `self_start_tx` protects records for the entity itself, but related traversal can still read relationship and entity-version rows associated with a previously deleted object that used the same integer ID. If the database reuses an ID, the newly created dashboard or chart could display activity belonging to its predecessor. Could we propagate the lower transaction boundary for the current entity into related-scope resolution and clip the relationship windows accordingly? Matching shadow records against the current UUID where available would provide additional protection. ### Minor documentation points - `PathEntityResponseError` and `resolve_endpoint_path_entity` in `superset/versioning/api_helpers.py:143` should begin with complete, single-line PEP 257 summaries. - The dated profiling reference in `superset/versioning/activity/queries.py:196` should use timeless wording while preserving the useful performance rationale. Overall, the architecture looks thoughtfully developed, but the chunk-ordering and entity-incarnation issues can affect the correctness of the activity stream and should be addressed before merge. -- 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]
