From 1d0b801af1ee9c7eec783d9f4648dbf12e7c38c7 Mon Sep 17 00:00:00 2001 From: "tom.bonfert" Date: Wed, 14 Oct 2026 09:39:47 +0200 Subject: [PATCH 1/6] feat(reporting): enforce unique event names and identity-based event 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. --- docs/impulse/docs/references/report/event.md | 5 +- skills/impulse-events/SKILL.md | 4 +- skills/impulse-reporting/SKILL.md | 3 +- src/impulse_reporting/core/report.py | 53 ++++-- .../unit/core/report_test.py | 179 ++++++++++++++++++ 5 files changed, 223 insertions(+), 21 deletions(-) diff --git a/docs/impulse/docs/references/report/event.md b/docs/impulse/docs/references/report/event.md index fa4671be..a79db01f 100644 --- a/docs/impulse/docs/references/report/event.md +++ b/docs/impulse/docs/references/report/event.md @@ -9,7 +9,8 @@ Events define time windows within measurement data that scope downstream aggrega time-series recordings into meaningful intervals -- for example, "engine RPM between 2000 and 5000". Every event used by an aggregation **must** be registered with the report via `add_event()` before `determine_report()` -is called. +is called. Event names must be unique within a report, and an aggregation must be scoped to the registered event +object itself, not to another object with the same name. ```python my_report.add_event(my_event) @@ -296,7 +297,7 @@ Stores event definitions (one row per event per report). | Column | Type | Description | |---------------------|---------------------|-----------------------------------------------------------------------------| -| `event_id` | `int` | Unique event identifier (CRC32 hash of name + expression). | +| `event_id` | `int` | Unique event identifier (CRC32 hash of the event name). | | `report_id` | `int` | Report identifier. | | `event_type` | `str` | `"BASIC_EVENT"`, `"CONTAINER_EVENT"`, `"SEQUENCE_OF_EVENTS"`, `"POINTS_IN_TIME_EVENT"`, or `"TIME_WINDOW_EVENT"`. | | `event_name` | `str` | Event name. | diff --git a/skills/impulse-events/SKILL.md b/skills/impulse-events/SKILL.md index 542caddf..bdce7201 100644 --- a/skills/impulse-events/SKILL.md +++ b/skills/impulse-events/SKILL.md @@ -16,7 +16,9 @@ An event defines time windows within a recording that scope downstream aggregati expression contains a UDF that declares `container_tags` / `container_metrics`, those values are injected as usual — the requirement propagates through the event. -**Every event used by an aggregation must be registered with the report before computing:** +**Every event used by an aggregation must be registered with the report before computing.** Event +names must be unique within a report, and the aggregation must be scoped to that registered object +(not a re-created event with the same name): ```python report.add_event(my_event) diff --git a/skills/impulse-reporting/SKILL.md b/skills/impulse-reporting/SKILL.md index 65014f20..c38e06f2 100644 --- a/skills/impulse-reporting/SKILL.md +++ b/skills/impulse-reporting/SKILL.md @@ -92,7 +92,8 @@ report.persist_results() `determine_report()` validates that **every event referenced by an aggregation has been registered with `add_event()`** before computing — otherwise it raises. Register events even when an aggregation -is the only thing that uses them. +is the only thing that uses them. The aggregation must reference the registered object itself (a +re-created event with the same name raises too), and `add_event()` rejects duplicate event names. After `determine_report()`, computed results are also available on the report object without a write — `report.aggregation_dfs[...]` (used by `impulse-ml`) — which is what makes sinkless mode useful. diff --git a/src/impulse_reporting/core/report.py b/src/impulse_reporting/core/report.py index 82955e8a..0da6bbb9 100644 --- a/src/impulse_reporting/core/report.py +++ b/src/impulse_reporting/core/report.py @@ -405,7 +405,9 @@ def add_event(self, event: Event): Raises ------ ValueError - If the event is a ContainerEvent and a ContainerEvent already exists in the report. + If the event is a ContainerEvent and a ContainerEvent already exists in the report, + or if an event with the same name has already been added (event names identify + events in the fact and dimension tables). """ if isinstance(event, ContainerEvent) and any( isinstance(e, ContainerEvent) for e in self.events @@ -414,6 +416,11 @@ def add_event(self, event: Event): "Only one ContainerEvent is allowed per report. " "A ContainerEvent has already been added to this report." ) + if any(e.get_name() == event.get_name() for e in self.events): + raise ValueError( + f"An event named '{event.get_name()}' has already been added to this report. " + "Event names must be unique within a report." + ) self.events.append(event) event.set_report_id(self.report_id) if isinstance(event, TimeWindowEvent): @@ -535,34 +542,46 @@ def _group_aggregations_by_type(self): def _validate_aggregation_events(self) -> None: """ - Validate that all events used in aggregations are added to the report. + Validate that every aggregation's event is an event object added to the report. + + The event must be the registered object itself, not another object with the same + name: the event fact is derived from the registered event, while the solve scopes the + aggregation to its own event, so a differing copy would produce mismatched rows. Raises ------ ValueError - If an aggregation uses an event that was not added to the report via add_event(). + If an aggregation uses an event that was not added to the report via add_event(), + or a different object with the name of a registered event. """ - registered_events = set(self.events) + registered_event_ids = {id(event) for event in self.events} registered_event_names = {event.get_name() for event in self.events} - missing_events = [] + unregistered_events = [] for page in self.pages: for aggregation in page.aggregations: event = aggregation.get_event() - if event is not None and event not in registered_events: - event_name = event.get_name() - if event_name not in registered_event_names: - missing_events.append( - f"Aggregation '{aggregation.get_name()}' uses event " - f"'{event_name}' which was not added to the report." - ) - - if missing_events: + if event is None or id(event) in registered_event_ids: + continue + event_name = event.get_name() + if event_name in registered_event_names: + unregistered_events.append( + f"Aggregation '{aggregation.get_name()}' uses an event named " + f"'{event_name}' that is a different object from the one added to " + "the report. Pass the registered event (e.g. " + f"report.get_events_dict()['{event_name}'])." + ) + else: + unregistered_events.append( + f"Aggregation '{aggregation.get_name()}' uses event " + f"'{event_name}' which was not added to the report." + ) + + if unregistered_events: error_message = ( - "The following events are used in aggregations but were not added " - "to the report via add_event():\n" - + "\n".join(f" - {msg}" for msg in missing_events) + "The following aggregations use events that were not added to the report " + "via add_event():\n" + "\n".join(f" - {msg}" for msg in unregistered_events) ) raise ValueError(error_message) diff --git a/tests/impulse_reporting/unit/core/report_test.py b/tests/impulse_reporting/unit/core/report_test.py index 7d067b3a..186f8f04 100644 --- a/tests/impulse_reporting/unit/core/report_test.py +++ b/tests/impulse_reporting/unit/core/report_test.py @@ -13,7 +13,9 @@ from impulse_reporting.core.page import Page from impulse_reporting.core.report import Report from impulse_reporting.events.basic_event import BasicEvent +from impulse_reporting.events.container_event import ContainerEvent from impulse_reporting.events.event_types import EventType +from impulse_reporting.events.time_window_event import TimeWindowEvent from impulse_reporting.persist.report_storage import SinkConfig DUMMY_CONFIG = { @@ -351,6 +353,88 @@ def test_create_solver_delta_with_solver_config(): assert solver.config.value_col == "value" +class TestAddEvent: + """Tests for add_event method.""" + + def test_duplicate_event_name_raises_error(self): + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + ts_expr = TimeSeriesSelector(TagSelector("name") == "test_signal") + report.add_event(BasicEvent(name="rpm_event", expr=ts_expr > 0)) + + with pytest.raises(ValueError, match="'rpm_event' has already been added"): + report.add_event(BasicEvent(name="rpm_event", expr=ts_expr > 100)) + + assert len(report.get_events()) == 1 + + def test_duplicate_event_name_of_other_type_raises_error(self): + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + ts_expr = TimeSeriesSelector(TagSelector("name") == "test_signal") + report.add_event(BasicEvent(name="window", expr=ts_expr > 0)) + + with pytest.raises(ValueError, match="'window' has already been added"): + report.add_event(TimeWindowEvent(name="window", window_length=60_000)) + + def test_same_event_added_twice_raises_error(self): + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + ts_expr = TimeSeriesSelector(TagSelector("name") == "test_signal") + event = BasicEvent(name="rpm_event", expr=ts_expr > 0) + report.add_event(event) + + with pytest.raises(ValueError, match="'rpm_event' has already been added"): + report.add_event(event) + + def test_second_container_event_keeps_container_event_error(self): + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + report.add_event(ContainerEvent(name="measurement")) + + with pytest.raises(ValueError, match="Only one ContainerEvent is allowed per report"): + report.add_event(ContainerEvent(name="measurement")) + + def test_distinct_event_names_are_all_registered(self): + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + ts_expr = TimeSeriesSelector(TagSelector("name") == "test_signal") + events = [ + BasicEvent(name="rpm_event", expr=ts_expr > 0), + ContainerEvent(name="measurement"), + TimeWindowEvent(name="window", window_length=60_000), + ] + for event in events: + report.add_event(event) + + assert report.get_events() == events + assert report.get_events_dict() == {event.get_name(): event for event in events} + + class TestValidateAggregationEvents: """Tests for _validate_aggregation_events method.""" @@ -568,6 +652,101 @@ def test_same_event_used_in_multiple_aggregations(self): report._validate_aggregation_events() + def test_same_name_different_definition_event_raises_error(self): + # The event fact would tile 60 s windows while the solve tiles 10 s windows. + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + report.add_event(TimeWindowEvent(name="w", window_length=60_000)) + + ts_expr = TimeSeriesSelector(TagSelector("name") == "test_signal") + stats = StatsAggregator( + name="window_stats", + input_expressions=[ts_expr], + channel_names=["test_signal"], + statistics=["mean"], + event=TimeWindowEvent(name="w", window_length=10_000), + ) + + page = Page(page_number=1) + page.add_aggregation(stats) + report.add_page(page) + + with pytest.raises(ValueError) as exc_info: + report._validate_aggregation_events() + + error_msg = str(exc_info.value) + assert "window_stats" in error_msg + assert "'w' that is a different object" in error_msg + + def test_same_name_identical_copy_event_raises_error(self): + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + ts_expr = TimeSeriesSelector(TagSelector("name") == "test_signal") + report.add_event(BasicEvent(name="rpm_event", expr=ts_expr > 0)) + + hist = HistogramDuration( + name="test_histogram", + base_expr=ts_expr, + bins=[0.0, 100.0], + event=BasicEvent(name="rpm_event", expr=ts_expr > 0), + ) + + page = Page(page_number=1) + page.add_aggregation(hist) + report.add_page(page) + + with pytest.raises(ValueError, match="'rpm_event' that is a different object"): + report._validate_aggregation_events() + + def test_unregistered_and_copied_events_are_listed_together(self): + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + ts_expr = TimeSeriesSelector(TagSelector("name") == "test_signal") + report.add_event(BasicEvent(name="registered_event", expr=ts_expr > 0)) + + copied = StatsAggregator( + name="copied_stats", + input_expressions=[ts_expr], + channel_names=["test_signal"], + statistics=["min"], + event=BasicEvent(name="registered_event", expr=ts_expr > 0), + ) + unregistered = StatsAggregator( + name="unregistered_stats", + input_expressions=[ts_expr], + channel_names=["test_signal"], + statistics=["max"], + event=BasicEvent(name="other_event", expr=ts_expr > 0), + ) + + page = Page(page_number=1) + page.add_aggregation(copied) + page.add_aggregation(unregistered) + report.add_page(page) + + with pytest.raises(ValueError) as exc_info: + report._validate_aggregation_events() + + error_msg = str(exc_info.value) + assert "'copied_stats' uses an event named 'registered_event'" in error_msg + assert "'unregistered_stats' uses event 'other_event'" in error_msg + assert "not added to the report" in error_msg + class TestValidateFullRecalculationScope: """Tests for _validate_full_recalculation_scope method.""" From 24a20fc99116db9c0b38a742ee8be604a467599e Mon Sep 17 00:00:00 2001 From: "tom.bonfert" Date: Wed, 14 Oct 2026 10:45:36 +0200 Subject: [PATCH 2/6] test(reporting): update error message assertions for unregistered aggregation events Align unit tests with the refined validation error message that names both the aggregation and the missing event. --- tests/impulse_reporting/unit/core/report_test.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/impulse_reporting/unit/core/report_test.py b/tests/impulse_reporting/unit/core/report_test.py index 186f8f04..5c790712 100644 --- a/tests/impulse_reporting/unit/core/report_test.py +++ b/tests/impulse_reporting/unit/core/report_test.py @@ -466,7 +466,7 @@ def test_statistics_event_not_registered_raises_error(self): assert "test_event" in str(exc_info.value) assert "test_stats" in str(exc_info.value) - assert "not added to the report" in str(exc_info.value) + assert "uses event 'test_event' which was not added to the report" in str(exc_info.value) def test_histogram_event_not_registered_raises_error(self): report: Report = Report( @@ -744,8 +744,10 @@ def test_unregistered_and_copied_events_are_listed_together(self): error_msg = str(exc_info.value) assert "'copied_stats' uses an event named 'registered_event'" in error_msg - assert "'unregistered_stats' uses event 'other_event'" in error_msg - assert "not added to the report" in error_msg + assert ( + "'unregistered_stats' uses event 'other_event' which was not added to the report" + in error_msg + ) class TestValidateFullRecalculationScope: From 30dc57d174f380d9c2fd8f221e9b291c8ee37d75 Mon Sep 17 00:00:00 2001 From: "tom.bonfert" Date: Wed, 14 Oct 2026 10:48:23 +0200 Subject: [PATCH 3/6] feat(reporting): make event name uniqueness case-insensitive 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. --- docs/impulse/docs/references/report/event.md | 4 +-- skills/impulse-events/SKILL.md | 4 +-- skills/impulse-reporting/SKILL.md | 3 +- src/impulse_reporting/core/report.py | 16 +++++++--- .../unit/core/report_test.py | 32 +++++++++++++++++++ 5 files changed, 49 insertions(+), 10 deletions(-) diff --git a/docs/impulse/docs/references/report/event.md b/docs/impulse/docs/references/report/event.md index a79db01f..0f34764a 100644 --- a/docs/impulse/docs/references/report/event.md +++ b/docs/impulse/docs/references/report/event.md @@ -9,8 +9,8 @@ Events define time windows within measurement data that scope downstream aggrega time-series recordings into meaningful intervals -- for example, "engine RPM between 2000 and 5000". Every event used by an aggregation **must** be registered with the report via `add_event()` before `determine_report()` -is called. Event names must be unique within a report, and an aggregation must be scoped to the registered event -object itself, not to another object with the same name. +is called. Event names must be unique within a report (ignoring case), and an aggregation must be scoped to the +registered event object itself, not to another object with the same name. ```python my_report.add_event(my_event) diff --git a/skills/impulse-events/SKILL.md b/skills/impulse-events/SKILL.md index bdce7201..3736a094 100644 --- a/skills/impulse-events/SKILL.md +++ b/skills/impulse-events/SKILL.md @@ -17,8 +17,8 @@ expression contains a UDF that declares `container_tags` / `container_metrics`, injected as usual — the requirement propagates through the event. **Every event used by an aggregation must be registered with the report before computing.** Event -names must be unique within a report, and the aggregation must be scoped to that registered object -(not a re-created event with the same name): +names must be unique within a report (ignoring case), and the aggregation must be scoped to that +registered object (not a re-created event with the same name): ```python report.add_event(my_event) diff --git a/skills/impulse-reporting/SKILL.md b/skills/impulse-reporting/SKILL.md index c38e06f2..fe3030c3 100644 --- a/skills/impulse-reporting/SKILL.md +++ b/skills/impulse-reporting/SKILL.md @@ -93,7 +93,8 @@ report.persist_results() `determine_report()` validates that **every event referenced by an aggregation has been registered with `add_event()`** before computing — otherwise it raises. Register events even when an aggregation is the only thing that uses them. The aggregation must reference the registered object itself (a -re-created event with the same name raises too), and `add_event()` rejects duplicate event names. +re-created event with the same name raises too), and `add_event()` rejects duplicate event names, +ignoring case. After `determine_report()`, computed results are also available on the report object without a write — `report.aggregation_dfs[...]` (used by `impulse-ml`) — which is what makes sinkless mode useful. diff --git a/src/impulse_reporting/core/report.py b/src/impulse_reporting/core/report.py index 0da6bbb9..34c3b102 100644 --- a/src/impulse_reporting/core/report.py +++ b/src/impulse_reporting/core/report.py @@ -406,8 +406,9 @@ def add_event(self, event: Event): ------ ValueError If the event is a ContainerEvent and a ContainerEvent already exists in the report, - or if an event with the same name has already been added (event names identify - events in the fact and dimension tables). + or if an event with the same name, ignoring case, has already been added (event + names identify events in the fact and dimension tables and name the event's + column in the solved data, which Spark resolves case-insensitively). """ if isinstance(event, ContainerEvent) and any( isinstance(e, ContainerEvent) for e in self.events @@ -416,10 +417,15 @@ def add_event(self, event: Event): "Only one ContainerEvent is allowed per report. " "A ContainerEvent has already been added to this report." ) - if any(e.get_name() == event.get_name() for e in self.events): + # lower() rather than casefold() mirrors Spark's case-insensitive column resolution. + existing = next( + (e for e in self.events if e.get_name().lower() == event.get_name().lower()), None + ) + if existing is not None: raise ValueError( - f"An event named '{event.get_name()}' has already been added to this report. " - "Event names must be unique within a report." + f"Cannot add event '{event.get_name()}': an event named " + f"'{existing.get_name()}' has already been added to this report. " + "Event names must be unique within a report, ignoring case." ) self.events.append(event) event.set_report_id(self.report_id) diff --git a/tests/impulse_reporting/unit/core/report_test.py b/tests/impulse_reporting/unit/core/report_test.py index 5c790712..6361dc5a 100644 --- a/tests/impulse_reporting/unit/core/report_test.py +++ b/tests/impulse_reporting/unit/core/report_test.py @@ -401,6 +401,38 @@ def test_same_event_added_twice_raises_error(self): with pytest.raises(ValueError, match="'rpm_event' has already been added"): report.add_event(event) + def test_duplicate_event_name_ignoring_case_raises_error(self): + # Spark resolves the solved event columns case-insensitively. + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + ts_expr = TimeSeriesSelector(TagSelector("name") == "test_signal") + report.add_event(BasicEvent(name="RPM_high", expr=ts_expr > 0)) + + with pytest.raises( + ValueError, match="Cannot add event 'rpm_high': an event named 'RPM_high'" + ): + report.add_event(BasicEvent(name="rpm_high", expr=ts_expr > 0)) + + def test_names_equal_only_under_casefold_are_distinct(self): + # Spark keeps these columns apart, so casefold() ("ß" == "ss") would over-reject. + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + ts_expr = TimeSeriesSelector(TagSelector("name") == "test_signal") + report.add_event(BasicEvent(name="Drehzahl_groß", expr=ts_expr > 0)) + report.add_event(BasicEvent(name="Drehzahl_gross", expr=ts_expr > 0)) + + assert len(report.get_events()) == 2 + def test_second_container_event_keeps_container_event_error(self): report: Report = Report( name="my_report", From 49773266808a86c21f5768d17ff793eca689412a Mon Sep 17 00:00:00 2001 From: "tom.bonfert" Date: Wed, 14 Oct 2026 11:02:04 +0200 Subject: [PATCH 4/6] docs(reporting): clarify case-insensitive event name uniqueness and aggregation 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. --- .../docs/references/api/impulse_reporting/core/report.md | 5 ++++- docs/impulse/docs/references/report/index.md | 5 +++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/impulse/docs/references/api/impulse_reporting/core/report.md b/docs/impulse/docs/references/api/impulse_reporting/core/report.md index 450b2970..3d4a8fb2 100644 --- a/docs/impulse/docs/references/api/impulse_reporting/core/report.md +++ b/docs/impulse/docs/references/api/impulse_reporting/core/report.md @@ -238,7 +238,10 @@ Add an event to the report. **Raises**: -- `ValueError`: If the event is a ContainerEvent and a ContainerEvent already exists in the report. +- `ValueError`: If the event is a ContainerEvent and a ContainerEvent already exists in the report, +or if an event with the same name, ignoring case, has already been added (event +names identify events in the fact and dimension tables and name the event's +column in the solved data, which Spark resolves case-insensitively). **Returns**: diff --git a/docs/impulse/docs/references/report/index.md b/docs/impulse/docs/references/report/index.md index a5087fa0..9fc0e3ed 100644 --- a/docs/impulse/docs/references/report/index.md +++ b/docs/impulse/docs/references/report/index.md @@ -40,7 +40,7 @@ Either `config` or `config_path` must be provided. | `get_solver()` | Returns the active `QuerySolver`. | -- | | `get_sink_config()` | Returns the active `SinkConfig` (e.g. `UnitySinkConfig` with `catalog_name`, `schema_name`, `table_prefix`). | -- | | `add_page(page)` | Adds a `Page` to the report. | `page`: `Page` instance. | -| `add_event(event)` | Registers an `Event` with the report. All events used by aggregations must be registered. | `event`: `Event` instance. | +| `add_event(event)` | Registers an `Event` with the report. All events used by aggregations must be registered. Event names must be unique (ignoring case). | `event`: `Event` instance. | | `add_calculated_channel(channel)` | Registers a [`CalculatedChannel`](./channel.md) with the report. | `channel`: `CalculatedChannel` instance. | | `get_calculated_channels()` | Returns the list of registered calculated channels. | -- | | `determine_report(is_incremental)` | Computes all events, aggregations, calculated channels, and container dimensions. Results are stored on the report object. | `is_incremental`: `bool` or `None`. Mode hint; overridden by `config.incremental` when present. See [Incremental processing](#incremental-processing). | @@ -62,7 +62,8 @@ my_report.persist_results() ``` `determine_report()` validates that every event referenced by an aggregation has been registered with `add_event()` -before computation begins. +before computation begins. The aggregation must reference the registered event object itself; a re-created event with +the same name is rejected. ### Sinkless mode From 4b20a4cb24b8e8843a47a9e3cc22ba82287b5b73 Mon Sep 17 00:00:00 2001 From: "tom.bonfert" Date: Wed, 14 Oct 2026 11:11:39 +0200 Subject: [PATCH 5/6] fix(reporting): make aggregation event validation case-insensitive 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. --- src/impulse_reporting/core/report.py | 22 +++++++------ .../unit/core/report_test.py | 33 +++++++++++++++++++ 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/src/impulse_reporting/core/report.py b/src/impulse_reporting/core/report.py index 34c3b102..93529270 100644 --- a/src/impulse_reporting/core/report.py +++ b/src/impulse_reporting/core/report.py @@ -558,30 +558,32 @@ def _validate_aggregation_events(self) -> None: ------ ValueError If an aggregation uses an event that was not added to the report via add_event(), - or a different object with the name of a registered event. + or a different object with the name (ignoring case) of a registered event. """ - registered_event_ids = {id(event) for event in self.events} - registered_event_names = {event.get_name() for event in self.events} + # Keyed like add_event's uniqueness check, so there is one event per key. + registered_events = {event.get_name().lower(): event for event in self.events} unregistered_events = [] for page in self.pages: for aggregation in page.aggregations: event = aggregation.get_event() - if event is None or id(event) in registered_event_ids: + if event is None: continue - event_name = event.get_name() - if event_name in registered_event_names: + registered = registered_events.get(event.get_name().lower()) + if registered is event: + continue + if registered is not None: unregistered_events.append( f"Aggregation '{aggregation.get_name()}' uses an event named " - f"'{event_name}' that is a different object from the one added to " - "the report. Pass the registered event (e.g. " - f"report.get_events_dict()['{event_name}'])." + f"'{event.get_name()}' that is a different object from the registered " + f"event '{registered.get_name()}'. Pass the registered event (e.g. " + f"report.get_events_dict()[{registered.get_name()!r}])." ) else: unregistered_events.append( f"Aggregation '{aggregation.get_name()}' uses event " - f"'{event_name}' which was not added to the report." + f"'{event.get_name()}' which was not added to the report." ) if unregistered_events: diff --git a/tests/impulse_reporting/unit/core/report_test.py b/tests/impulse_reporting/unit/core/report_test.py index 6361dc5a..f56b2750 100644 --- a/tests/impulse_reporting/unit/core/report_test.py +++ b/tests/impulse_reporting/unit/core/report_test.py @@ -740,6 +740,39 @@ def test_same_name_identical_copy_event_raises_error(self): with pytest.raises(ValueError, match="'rpm_event' that is a different object"): report._validate_aggregation_events() + def test_name_differing_only_by_case_counts_as_copy(self): + report: Report = Report( + name="my_report", + spark=None, + workspace_client=create_autospec(WorkspaceClient), + config=DUMMY_CONFIG, + ) + + ts_expr = TimeSeriesSelector(TagSelector("name") == "test_signal") + report.add_event(BasicEvent(name="RPM_high", expr=ts_expr > 0)) + + stats = StatsAggregator( + name="rpm_stats", + input_expressions=[ts_expr], + channel_names=["test_signal"], + statistics=["mean"], + event=BasicEvent(name="rpm_high", expr=ts_expr > 0), + ) + + page = Page(page_number=1) + page.add_aggregation(stats) + report.add_page(page) + + with pytest.raises(ValueError) as exc_info: + report._validate_aggregation_events() + + error_msg = str(exc_info.value) + assert ( + "'rpm_high' that is a different object from the registered event 'RPM_high'" + in error_msg + ) + assert "report.get_events_dict()['RPM_high']" in error_msg + def test_unregistered_and_copied_events_are_listed_together(self): report: Report = Report( name="my_report", From ef2f0040efc24c84b6c93e828ca2599923fcab91 Mon Sep 17 00:00:00 2001 From: "tom.bonfert" Date: Wed, 14 Oct 2026 11:32:33 +0200 Subject: [PATCH 6/6] docs(reporting): refine comments and error messages for event name uniqueness 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. --- src/impulse_reporting/core/report.py | 6 +++--- tests/impulse_reporting/unit/core/report_test.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/impulse_reporting/core/report.py b/src/impulse_reporting/core/report.py index 93529270..1a6b6f9f 100644 --- a/src/impulse_reporting/core/report.py +++ b/src/impulse_reporting/core/report.py @@ -417,7 +417,7 @@ def add_event(self, event: Event): "Only one ContainerEvent is allowed per report. " "A ContainerEvent has already been added to this report." ) - # lower() rather than casefold() mirrors Spark's case-insensitive column resolution. + # Names are compared like Spark's case-insensitive column resolution. existing = next( (e for e in self.events if e.get_name().lower() == event.get_name().lower()), None ) @@ -588,8 +588,8 @@ def _validate_aggregation_events(self) -> None: if unregistered_events: error_message = ( - "The following aggregations use events that were not added to the report " - "via add_event():\n" + "\n".join(f" - {msg}" for msg in unregistered_events) + "The following aggregations reference events that are not registered with " + "this report:\n" + "\n".join(f" - {msg}" for msg in unregistered_events) ) raise ValueError(error_message) diff --git a/tests/impulse_reporting/unit/core/report_test.py b/tests/impulse_reporting/unit/core/report_test.py index f56b2750..a3e9fb5b 100644 --- a/tests/impulse_reporting/unit/core/report_test.py +++ b/tests/impulse_reporting/unit/core/report_test.py @@ -418,8 +418,8 @@ def test_duplicate_event_name_ignoring_case_raises_error(self): ): report.add_event(BasicEvent(name="rpm_high", expr=ts_expr > 0)) - def test_names_equal_only_under_casefold_are_distinct(self): - # Spark keeps these columns apart, so casefold() ("ß" == "ss") would over-reject. + def test_names_distinct_in_spark_are_allowed(self): + # Spark resolves these as two distinct columns, so both names are accepted. report: Report = Report( name="my_report", spark=None,