fix(cohorts): consult correct fields for time windows#66394
Merged
Conversation
Contributor
|
Reviews (1): Last reviewed commit: "consult correct fields + tests" | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Bug fix corrects performed_event_multiple to consult explicit_datetime fields (mirroring the single-event path) by delegating to the existing explicit_eviction_window helper. Logic is isolated, reuses proven code, and is covered by 7 new parametrized tests that verify edge cases and parity with the time_value/time_interval path.
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
A realtime behavioral cohort like
OR(performed_event_multiple $pageview gte 3 [last 7 days], person email is_set)loaded but emitted nothing to the shadow topic.The cohort UI stores a relative window such as "last 7 days" as
explicit_datetime: "-7d"with notime_interval/time_value, but theperformed_event_multiplewindow resolvereffective_window_daysread onlytime_interval/time_value.With no interval it returned
0, which maps toHourlyDeferred, drops the leaf, and collapses the whole cohort toExcluded(HasDroppedLeaf).The single
performed_eventpath already consultedexplicit_datetime, so only_multiplediverged from the production oracle.The service in only deployed in the dev environment for now.
Changes
effective_window_daysnow consultsexplicit_datetime(_to) first, reusing the single path'sexplicit_eviction_windowclassifier via a newexplicit_window_dayshelper.A relative lower only bound (
"-7d","-1y") resolves to its sliding window; every other explicit shape (sub day, absolute range, two sided/relative upper, unparseable) still returns0and drops, exactly as before.The signature stays
u32, so both call sites are untouched, and state keys already discriminateexplicit_datetime, so no key change is needed.How did you test this code?
Automated unit tests:
pick_state.rscovers window day routing and parity with thetime_value/time_intervalpath.leaf_classifier.rscovers Keep/Drop classification of explicit_multipleleaves.reverse_index.rscovers the frozenwindow_days/predicate_opmetadata and the same parity.cargo test -p cohort-stream-processor --libpasses 737 tests andcargo clippyis clean.