fix: Filtering for list context and experiments#924
Conversation
Changed Files
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR refactors dimension-based filtering across three files by capturing original request dimension keys before evaluation and passing them as references instead of owned strings. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates dimension-based filtering used by list endpoints so filtering is driven by the original requested dimension keys (pre local-cohort evaluation), and relaxes the previous length-based constraint.
Changes:
- Adjust
Contextual::filter_by_dimensionsignature/logic to iterate over original request dimension keys and remove therequest_keys_len <= variables.len()condition. - Update context and experiment list handlers to pass original request keys into
filter_by_dimension. - Update unit tests for the new
filter_by_dimensionsignature.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
crates/superposition_types/src/contextual.rs |
Changes filter_by_dimension API/logic and updates tests accordingly. |
crates/experimentation_platform/src/api/experiments/handlers.rs |
Passes original request keys into experiment list filtering pipeline. |
crates/context_aware_config/src/api/context/handlers.rs |
Passes original request keys into context list filtering pipeline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn filter_by_dimension( | ||
| contexts: Vec<Self>, | ||
| dimension_keys: &[String], | ||
| request_keys_len: usize, | ||
| original_dimension_keys: &[&String], | ||
| dimensions_info: &HashMap<String, DimensionInfo>, |
There was a problem hiding this comment.
filter_by_dimension now takes &[&String], which forces callers to keep the backing map/vec alive for the duration of filtering and makes call sites much harder to write correctly (and can easily lead to borrow-checker issues when the source map is later shadowed/mutated). Consider changing this parameter to an owned/key-like type (e.g., &[String] or &[&str]) so callers can pass cloned keys or string slices without tying lifetimes to the original request map.
| let original_req_keys = dimension_params.keys().collect::<Vec<_>>(); | ||
| let dimension_params = evaluate_local_cohorts_skip_unresolved( | ||
| &dimensions_info, | ||
| &dimension_params, | ||
| ); |
There was a problem hiding this comment.
original_req_keys is collected as Vec<&String> from dimension_params.keys(), and then dimension_params is immediately shadowed by the evaluated map. The shadowing drops the original dimension_params, so the references inside original_req_keys can’t remain valid; this should fail to compile. Fix by collecting owned keys (e.g., clone into Vec<String>) or avoid shadowing by storing the evaluated map in a separate variable so the original map outlives original_req_keys.
| let dimensions_info = | ||
| fetch_dimensions_info_map(&mut conn, &workspace_context.schema_name)?; | ||
| let original_request_len = dimension_params.len(); | ||
| let original_req_keys = dimension_params.keys().collect::<Vec<_>>(); |
There was a problem hiding this comment.
original_req_keys is collected as Vec<&String> from dimension_params.keys(), and then dimension_params is shadowed with the evaluated map. That shadowing drops the original dimension_params, so original_req_keys ends up holding invalid references (this should be rejected by the borrow checker). Fix by cloning the keys into an owned Vec<String> or by keeping the original dimension_params binding alive (store the evaluated result in a different variable name).
| let original_req_keys = dimension_params.keys().collect::<Vec<_>>(); | |
| let original_req_keys = dimension_params.keys().cloned().collect::<Vec<String>>(); |
Changelog
Fix of fix
Summary by CodeRabbit