-
Notifications
You must be signed in to change notification settings - Fork 449
feat: Add explicit ordering for segment rule conditions #5671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add explicit ordering for segment rule conditions #5671
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Docker builds report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM without a comment typo.
I think you mentioned the possibility of making SEGMENT_RULES_CONDITIONS_EXPLICIT_ORDERING_ENABLED = True
default for new users. Is that something we'd like to pursue in this PR?
Co-authored-by: Evandro Myller <[email protected]>
Not in this PR, as it's intended to quickly unblock a customer. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5671 +/- ##
=======================================
Coverage 97.76% 97.76%
=======================================
Files 1245 1245
Lines 44062 44071 +9
=======================================
+ Hits 43076 43085 +9
Misses 986 986 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
376f5ec
to
4ea6956
Compare
4ea6956
to
b1644d3
Compare
b1644d3
to
d7eb88e
Compare
e4bb950
to
1be4c5a
Compare
Thanks for submitting a PR! Please check the boxes below:
docs/
if required so people know about the feature!Changes
This adds a
SEGMENT_RULES_CONDITIONS_EXPLICIT_ORDERING_ENABLED
setting to forceORDER BY id ASC
clause to all segment condition selects, which remedies the existing problem with no guaranteed ordering (#5669).How did you test this code?
Modified the existing view test to use the setting and avoid the explicit test data result ordering, which obviously masked the problem in the first place.