Use xxHash64 for event_instance_id to prevent hash collisions - #85
Merged
Conversation
…t schema to LongType Replaces the CRC32-based event_instance_id generation with xxHash64 to reduce collisions at scale, and updates `event_instance_id` in `EVENT_INSTANCE_FACT_SCHEMA` from `IntegerType` to `LongType` to hold the signed 64-bit hash values. ContainerEvent rows continue to hash only `container_id`, while other event types hash `container_id::event_name::start_ts::end_ts`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
=======================================
Coverage 89.01% 89.01%
=======================================
Files 61 61
Lines 5207 5207
Branches 628 628
=======================================
Hits 4635 4635
Misses 461 461
Partials 111 111
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Replaces the non-negative CRC32 assertion with checks that every BasicEvent instance has an event_instance_id and that all instance ids are distinct, matching the switch to per-instance timestamp-based xxHash64 hashing.
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
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.
Problem
event_instance_idwas generated with Spark'scrc32, whose 32-bit output space produces collisions in practice. A user hit a real case where two distinct instances of the same event on one container (sameevent_name, different time intervals) hashed to the same id.Because
event_instance_idis a MERGE key forevent_instance_factand the join key fromstats_aggregator_factandpoint_value_fact, a collision silently drops or mis-associates event instances in the gold layer.Fix
Switch
event_instance_idgeneration fromcrc32toxxhash64(64-bit), widening the key space enough to keep it collision-free at scale. The change is made in the single shared helper (generate_event_instance_id_column), soevent_instance_factand the aggregation facts stay in lockstep automatically.Changes
event_instance_util.py:crc32toxxhash64for both the container-event and timestamp-based branches; docstring updated.fact_schema.py:event_instance_idtyped asLongTypeto match the 64-bit hash andSTATS_AGGREGATOR_FACT_SCHEMA(cosmetic; persistence projects by name).stats_aggregator.pyupdated to mirror the new hash.Notes
event_instance_idvalues are now 64-bit and can be negative. Equality still holds for MERGE and joins, so correctness is unaffected, but any downstream consumer assuming a positive or 32-bit id should be aware.Verification
Targeted unit tests, the events/aggregations/incremental sweep, and the statistics integration tests pass;
make lintclean.