Repository navigation
fix(reporting): reject duplicate event names and same-name event copies in aggregations - #121
Merged
Merged
Conversation
…registration - `Report.add_event()` now rejects duplicate event names (including the same object added twice). - `_validate_aggregation_events()` now requires aggregations to reference the exact registered event object, not just an event with the same name. - Updated event and reporting docs/skills to clarify uniqueness and object-identity requirements. - Adjusted `event_id` documentation to reflect that it hashes only the event name. - Added unit tests covering duplicate-name rejection, same-object re-addition, and aggregation validation for copied/same-name events.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
+ Coverage 90.17% 90.21% +0.04%
==========================================
Files 66 66
Lines 6003 6009 +6
Branches 755 757 +2
==========================================
+ Hits 5413 5421 +8
+ Misses 467 466 -1
+ Partials 123 122 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…regation events Align unit tests with the refined validation error message that names both the aggregation and the missing event.
Event names are used as column names in solved data, which Spark resolves case-insensitively, so `Report.add_event()` now rejects duplicate event names ignoring case (using `lower()` to mirror Spark behavior). Added unit tests covering case-insensitive rejection and casefold-distinct names. Updated event and reporting docs/skills to clarify the case-insensitive uniqueness requirement.
…ggregation identity requirements Update API and report reference docs to state that `Report.add_event()` rejects duplicate event names ignoring case (because Spark resolves solved-data columns case-insensitively) and that aggregations must reference the exact registered event object, not a recreated event with the same name.
Update `_validate_aggregation_events()` to match aggregation events against registered events ignoring case, consistent with `add_event()` case-insensitive uniqueness. Error messages now reference the actual registered event name and suggest `report.get_events_dict()` with the correct key. Added a unit test for an aggregation event whose name differs only by case from a registered event.
…iqueness Clarify the comment explaining case-insensitive event name comparison in `Report.add_event()` and update the unregistered aggregation error message to be more precise. Rename the corresponding unit test to better express that names distinct in Spark are allowed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Event names identify events in the gold tables (
event_id = crc32(name)), butReportnever enforced them, so reports could produce colliding or mismatched fact rows without any error. Closes #113.add_eventnow rejects an event whose name is already registered, including the same object added twice. The existing "Only one ContainerEvent" check and its message are unchanged._validate_aggregation_eventsnow requires an aggregation's event to be the registered object itself. A different object with a registered name used to pass. It now raises and points toreport.get_events_dict()[name].Behavior change
Re-creating an event with the same name for an aggregation now raises, even if the copy is identical. This differs from the hash-equivalence exception proposed in the issue, because definition hashes leave out result-relevant fields (e.g.
required_channels) and a copy never gets the settingsadd_eventapplies (set_report_id,set_channel_time).Other changes
event.md,impulse-events,impulse-reporting): state the uniqueness and identity rule.event.md:event_idis the CRC32 of the event name only, not name + expression.Test Plan
Checklist