fix(transform): handle zero-duration base and sub events in merge_subwatcher_fields#141
Open
fanxing11 wants to merge 2 commits into
Open
fix(transform): handle zero-duration base and sub events in merge_subwatcher_fields#141fanxing11 wants to merge 2 commits into
fanxing11 wants to merge 2 commits into
Conversation
…watcher_fields Two related bugs in the boundary/intersection logic: 1. A zero-duration base event with an overlapping sub was silently dropped. base_period.duration == 0 makes boundary_points collapse to a single element, so the segment loop produces no output. Now preserved as a single zero-duration event enriched from the active sub. 2. A zero-duration sub event (instantaneous marker whose timestamp falls inside a base) was coloring the entire base event. Timeslot.intersection returns a zero-length but truthy result when a segment's boundary coincides with the instant, so every adjacent segment matched the sub and they merged back into one fully-enriched event. Now zero-length intersections on a non-instant base are ignored — a sub that was active for zero time can't enrich any slice of the base. Added 3 tests covering: zero-duration base + overlapping sub, zero-duration base + no overlap (locks in fast path), zero-duration sub + non-instant base.
Greptile SummaryThis PR fixes zero-duration interval handling in
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "fix(transform): handle zero-duration bas..." | Re-trigger Greptile |
The new instant-base branch reused 'best_sub' / 'best_sub_period' as plain local names, then the segment loop further down redeclares them with Optional[] type annotations. mypy treats this as 'Name already defined' even though control flow makes them disjoint. Renaming the instant-base variables to 'instant_best_*' satisfies mypy and makes the intent clearer.
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.
Found two related bugs in
merge_subwatcher_fieldswhile poking at the new transform. Both stem from how the boundary/intersection logic deals with zero-lengthTimeslots.Bug 1 — zero-duration base event is silently dropped
When the base is instantaneous and does overlap a sub,
boundary_pointscollapses to a single point, sozip(boundary_points, boundary_points[1:])is empty and nothing is emitted. The no-overlap fast path further up returns the base unchanged, so the symptom only shows up when there's a sub.Fix: dedicated branch for instant bases that picks the active sub via the same "latest sub wins" rule and returns a single enriched zero-duration event.
Bug 2 — instantaneous sub event "colors" the whole base
Expected: the sub is active for zero time → no slice should be enriched.
Root cause:
Timeslot([0,3]).intersection(Timeslot([3,3]))returns a zero-length but truthyTimeslot. The segment loop only checkedif not segment_period.intersection(sub_period): continue, so every segment adjacent to the instant matched the sub. Then the dedup pass merged the (now identical) segments back into one fully-colored event.Fix: ignore zero-length intersections both when collecting overlapping subs for a non-instant base and inside the per-segment matcher.
ip.duration > 0is the right rule — a sub that was active for zero time can't enrich any slice. (Instant base is still handled correctly, via the dedicated branch above.)Tests
Added three:
test_merge_subwatcher_fields_zero_duration_base_event_preserved— covers bug 1test_merge_subwatcher_fields_zero_duration_base_event_no_overlap_preserved— locks in the existing no-overlap fast path so future refactors don't regress ittest_merge_subwatcher_fields_zero_duration_sub_does_not_color_base— covers bug 2All 30
test_transformstests pass; ruff clean.Real-world relevance
Most watchers heartbeat with a nonzero pulsetime, so canonical queries probably don't hit either case today. But the function is exposed in query2 and via the Python API, and instantaneous events are a perfectly valid shape (manual API posts, edge transformations, future watchers reporting markers). Wanted to lock down the contract before this transform shows up in more pipelines.