feat: enhance rollup aggregation with dedup logic#1843
Conversation
fe923af to
de1d3d5
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves the tree_tests_rollup denormalized aggregation to correctly handle reprocessing when a test transitions from a NULL status to a concrete status, avoiding inflated totals by applying a “correction” delta (decrement null_tests, increment the concrete bucket, keep total_tests unchanged).
Changes:
- Added a rollup-specific processed-item key (
get_rollup_key) and usedProcessedListingItemsto deduplicate rollup processing and detectNULL -> non-NULLtransitions. - Extended rollup aggregation helpers to support correction deltas via
is_correction/reprocess_test_ids. - Added unit tests validating correction behavior and the
reprocess_test_idspathway.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backend/kernelCI_app/management/commands/process_pending_aggregations.py | Adds rollup key generation and dedup/correction detection using ProcessedListingItems during rollup batching. |
| backend/kernelCI_app/management/commands/helpers/process_pending_helpers.py | Implements correction delta behavior in rollup accumulation and threads reprocess_test_ids through aggregation. |
| backend/kernelCI_app/tests/unitTests/commands/process_pending_helpers_test.py | Adds unit tests covering null -> non-null correction behavior and reprocess_test_ids handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| continue | ||
| else: | ||
| # null -> non-null: This is a correction (reprocess) | ||
| reprocess_test_ids.add(test.test_id) |
There was a problem hiding this comment.
In the null->non-null correction path (stored_status is None and new test.status is not None), the code marks the test for reprocessing but does not add an updated ProcessedListingItems entry to set status to the new non-null value. This leaves the processed row with status=None permanently, so any future re-ingestion of the same test_id will be treated as a correction again, repeatedly decrementing null_tests and skewing rollup totals. Add/update a ProcessedListingItems entry for the correction case (same listing_item_key) so bulk_create(update_conflicts=True) updates status away from None once corrected.
| reprocess_test_ids.add(test.test_id) | |
| reprocess_test_ids.add(test.test_id) | |
| new_processed_entries.add( | |
| ProcessedListingItems( | |
| listing_item_key=rollup_key, | |
| checkout_id=checkout_id, | |
| status=test.status, | |
| ) | |
| ) |
| def aggregate_tests_rollup( | ||
| ready_tests: Sequence[PendingTest], | ||
| test_builds_by_id: dict[str, Builds], | ||
| issues_map: dict[str, dict], | ||
| reprocess_test_ids: set[str] = set(), | ||
| ) -> dict[tuple, dict]: |
There was a problem hiding this comment.
reprocess_test_ids uses a mutable default (set()). Even if this function doesn't mutate it today, mutable defaults are error-prone and make future edits risky. Prefer reprocess_test_ids: Optional[set[str]] = None and normalize to an empty set inside the function.
de1d3d5 to
8aaa613
Compare
| new_processed_entries: set[ProcessedListingItems] = set() | ||
|
|
||
| for test in ready_tests: | ||
| rollup_key = rollup_keys_by_test_id[test.test_id] |
There was a problem hiding this comment.
do we really need to precompute the rollup keys?
I believe we could compute them on demand (it would even avoid computation of keys where build id is not found)
There was a problem hiding this comment.
theoretically we should just receive tests here that have a build, the check below is mostly for checkout. we can move this after the check for sure, anyway
| record[counter] += 1 | ||
| else: | ||
| record[counter] += 1 | ||
| record["total_tests"] += 1 |
There was a problem hiding this comment.
wouldn´t it be safer (less error prone) to make total_tests a derived value?
I believe it should be the sum of a small amount of fields, and wouldn´t make a significant impact on performance.
There was a problem hiding this comment.
we could make it computed in the database, as we process tests individually and within batches, it would be kinda bad to have this computation here
maybe something like: https://www.postgresql.org/docs/current/ddl-generated-columns.html
MarceloRobert
left a comment
There was a problem hiding this comment.
LGTM. These things happen when we end up ingesting the same file multiple times such as when the ingester breaks for some reason
Description
Adds deduplication logic to the tree_tests_rollup aggregation and handle cases where a test's status is updated from null to a concrete value (pass/fail/etc.). Previously, these updates would incorrectly increment counters without adjusting the null count, leading to inflated totals.
Changes
How to test
Part of #1801