-
Notifications
You must be signed in to change notification settings - Fork 40
fix: exp and config partial apply #831
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,7 +11,10 @@ use std::{ | |||||||||||||||||||||||||||||||||||||||||
| cell::RefCell, | ||||||||||||||||||||||||||||||||||||||||||
| ffi::{c_int, CString}, | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
| use superposition_types::DimensionInfo; | ||||||||||||||||||||||||||||||||||||||||||
| use superposition_types::{ | ||||||||||||||||||||||||||||||||||||||||||
| logic::{evaluate_local_cohorts, evaluate_local_cohorts_skip_unresolved}, | ||||||||||||||||||||||||||||||||||||||||||
| DimensionInfo, | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
| use tokio::{runtime::Runtime, task}; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| thread_local! { | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -216,6 +219,7 @@ pub extern "C" fn expt_get_applicable_variant( | |||||||||||||||||||||||||||||||||||||||||
| #[no_mangle] | ||||||||||||||||||||||||||||||||||||||||||
| pub extern "C" fn expt_get_satisfied_experiments( | ||||||||||||||||||||||||||||||||||||||||||
| client: *mut Arc<Client>, | ||||||||||||||||||||||||||||||||||||||||||
| c_dimensions: *const c_char, | ||||||||||||||||||||||||||||||||||||||||||
| c_context: *const c_char, | ||||||||||||||||||||||||||||||||||||||||||
| filter_prefix: *const c_char, | ||||||||||||||||||||||||||||||||||||||||||
| ) -> *mut c_char { | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -227,6 +231,16 @@ pub extern "C" fn expt_get_satisfied_experiments( | |||||||||||||||||||||||||||||||||||||||||
| return std::ptr::null_mut() | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let dimensions = unwrap_safe!( | ||||||||||||||||||||||||||||||||||||||||||
| cstring_to_rstring(c_dimensions), | ||||||||||||||||||||||||||||||||||||||||||
| return std::ptr::null_mut() | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let dimensions = unwrap_safe!( | ||||||||||||||||||||||||||||||||||||||||||
| serde_json::from_str::<HashMap<String, DimensionInfo>>(dimensions.as_str()), | ||||||||||||||||||||||||||||||||||||||||||
| return std::ptr::null_mut() | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let prefix_list = if filter_prefix.is_null() { | ||||||||||||||||||||||||||||||||||||||||||
| None | ||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -238,6 +252,8 @@ pub extern "C" fn expt_get_satisfied_experiments( | |||||||||||||||||||||||||||||||||||||||||
| Some(prefix_list) | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let context = evaluate_local_cohorts(&dimensions, &context); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let local = task::LocalSet::new(); | ||||||||||||||||||||||||||||||||||||||||||
| local.block_on(&Runtime::new().unwrap(), async move { | ||||||||||||||||||||||||||||||||||||||||||
| unsafe { | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -258,6 +274,7 @@ pub extern "C" fn expt_get_satisfied_experiments( | |||||||||||||||||||||||||||||||||||||||||
| #[no_mangle] | ||||||||||||||||||||||||||||||||||||||||||
| pub extern "C" fn expt_get_filtered_satisfied_experiments( | ||||||||||||||||||||||||||||||||||||||||||
| client: *mut Arc<Client>, | ||||||||||||||||||||||||||||||||||||||||||
| c_dimensions: *const c_char, | ||||||||||||||||||||||||||||||||||||||||||
| c_context: *const c_char, | ||||||||||||||||||||||||||||||||||||||||||
| filter_prefix: *const c_char, | ||||||||||||||||||||||||||||||||||||||||||
| ) -> *mut c_char { | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -268,6 +285,17 @@ pub extern "C" fn expt_get_filtered_satisfied_experiments( | |||||||||||||||||||||||||||||||||||||||||
| serde_json::from_str::<Map<String, Value>>(context.as_str()), | ||||||||||||||||||||||||||||||||||||||||||
| return std::ptr::null_mut() | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let dimensions = unwrap_safe!( | ||||||||||||||||||||||||||||||||||||||||||
| cstring_to_rstring(c_dimensions), | ||||||||||||||||||||||||||||||||||||||||||
| return std::ptr::null_mut() | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let dimensions = unwrap_safe!( | ||||||||||||||||||||||||||||||||||||||||||
| serde_json::from_str::<HashMap<String, DimensionInfo>>(dimensions.as_str()), | ||||||||||||||||||||||||||||||||||||||||||
| return std::ptr::null_mut() | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+289
to
+297
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same null check issue for Apply the same null check fix here as well. 🐛 Proposed fix+ null_check!(c_dimensions, "Dimensions cannot be a null string", return std::ptr::null_mut());
+
let dimensions = unwrap_safe!(
cstring_to_rstring(c_dimensions),
return std::ptr::null_mut()
);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let prefix_list = if filter_prefix.is_null() { | ||||||||||||||||||||||||||||||||||||||||||
| None | ||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -280,6 +308,9 @@ pub extern "C" fn expt_get_filtered_satisfied_experiments( | |||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Some(prefix_list).filter(|list| !list.is_empty()) | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let context = evaluate_local_cohorts_skip_unresolved(&dimensions, &context); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let local = task::LocalSet::new(); | ||||||||||||||||||||||||||||||||||||||||||
| local.block_on(&Runtime::new().unwrap(), async move { | ||||||||||||||||||||||||||||||||||||||||||
| unsafe { | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -194,7 +194,7 @@ pub fn get_satisfied_experiments( | |
| ) -> Result<Experiments, String> { | ||
| let running_experiments = experiments | ||
| .iter() | ||
| .filter(|exp| superposition_types::partial_apply(&exp.context, context)) | ||
| .filter(|exp| superposition_types::apply(&exp.context, context)) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
|
|
@@ -208,6 +208,33 @@ pub fn get_satisfied_experiments( | |
| Ok(running_experiments) | ||
| } | ||
|
|
||
| pub fn get_filtered_satisfied_experiments( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Function needs a better name - it implies that
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Datron this is what it was called in the exp_client crate So i added this in this PR |
||
| experiments: &Experiments, | ||
| context: &Map<String, Value>, | ||
| filter_prefixes: Option<Vec<String>>, | ||
| ) -> Result<Experiments, String> { | ||
| let running_experiments = experiments | ||
| .iter() | ||
| .filter_map(|exp| { | ||
| if exp.context.is_empty() { | ||
| Some(exp.clone()) | ||
| } else { | ||
| superposition_types::partial_apply(&exp.context, context) | ||
| .then(|| exp.clone()) | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| if let Some(prefix_list) = filter_prefixes { | ||
| return Ok(filter_experiments_by_prefix( | ||
| running_experiments, | ||
| prefix_list, | ||
| )); | ||
| } | ||
|
|
||
| Ok(running_experiments) | ||
| } | ||
|
|
||
| fn filter_experiments_by_prefix( | ||
| experiments: Experiments, | ||
| filter_prefixes: Vec<String>, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ use crate::{ | |
| DimensionInfo, | ||
| }; | ||
|
|
||
| #[inline] | ||
| fn apply_logic( | ||
| condition: &Map<String, Value>, | ||
| context: &Map<String, Value>, | ||
|
|
@@ -37,15 +38,42 @@ fn apply_logic( | |
| true | ||
| } | ||
|
|
||
| /// Core context application logic - checks if all dimensions in condition are satisfied by context | ||
| /// Only exact matches are considered valid, except for "variantIds" dimension where containment is checked | ||
| /// Returns true if condition is satisfied by context, false otherwise | ||
| pub fn apply(condition: &Map<String, Value>, context: &Map<String, Value>) -> bool { | ||
| apply_logic(condition, context, false) | ||
| } | ||
|
|
||
| /// Filtering logic that allows partial matching of context | ||
| /// For dimensions present in context, performs the same checks as `apply` | ||
| /// For dimensions absent in context, skips the check (allows partial matching) | ||
| /// This is useful for matching contexts that may not have all dimensions defined | ||
| /// For array context values, checks for containment - added behaviour over `apply` | ||
| /// Returns false if there is a mismatch, true otherwise | ||
| pub fn partial_apply( | ||
| condition: &Map<String, Value>, | ||
| context: &Map<String, Value>, | ||
| ) -> bool { | ||
| apply_logic(condition, context, true) | ||
| for (dimension, value) in condition { | ||
| let Some(context_value) = context.get(dimension) else { | ||
| continue; // Skip dimensions not in context (partial matching) | ||
| }; | ||
|
|
||
| // For array context values, check containment | ||
| if let Value::Array(context_values) = context_value { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ayushjain17 - the input in the handler is a JSON stringified array or CSV ? You showed JSON stringified array. In some other places we do repeated values of the same key and collect them in the handler. Just asking if it is all consistent in how we get multi-value input in the handler?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to verify once which all spec are supported Input spec is not changed via this change-log Here, updating the internal logic only
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the spec for these APIs are different, they are un-typed json repeating values with same key is only possible for types which have type |
||
| if !context_values.contains(value) { | ||
| return false; | ||
| } | ||
| } else if dimension == "variantIds" { | ||
| // variantIds must always be an array - fail if scalar | ||
| return false; | ||
| } else if *context_value != *value { | ||
| // For non-array values, check equality | ||
| return false; | ||
| } | ||
| } | ||
| true | ||
| } | ||
|
|
||
| fn _evaluate_local_cohort_dimension( | ||
|
|
@@ -141,16 +169,10 @@ fn evaluate_local_cohorts_dependency( | |
| } | ||
| } | ||
|
|
||
| /// Evaluates all local cohort dimensions based on the provided query data and dimension definitions | ||
| /// First all local cohorts which are computable from the query data are evaluated, then any remaining local cohorts are set to "otherwise" | ||
| /// Computation starts from such a point, such that dependencies can be resolved in a depth-first manner | ||
| /// | ||
| /// Values of regular and remote cohort dimensions in query_data are retained as is. | ||
| /// Returned value, might have a different value for local cohort dimensions based on its based on dimensions, | ||
| /// if the value provided for the local cohort was incorrect in the query data. | ||
| pub fn evaluate_local_cohorts( | ||
| fn _evaluate_local_cohorts( | ||
| dimensions: &HashMap<String, DimensionInfo>, | ||
| query_data: &Map<String, Value>, | ||
| skip_unresolved: bool, | ||
| ) -> Map<String, Value> { | ||
| if dimensions.is_empty() { | ||
| return query_data.clone(); | ||
|
|
@@ -175,6 +197,10 @@ pub fn evaluate_local_cohorts( | |
| } | ||
| } | ||
|
|
||
| if skip_unresolved { | ||
| return modified_context; | ||
| } | ||
|
|
||
| // For any local cohort dimension not yet set, set it to "otherwise" | ||
| for dimension_key in dimensions.keys() { | ||
| if let Some(dimension_info) = dimensions.get(dimension_key) { | ||
|
|
@@ -192,6 +218,28 @@ pub fn evaluate_local_cohorts( | |
| modified_context | ||
| } | ||
|
|
||
| /// Evaluates all local cohort dimensions based on the provided query data and dimension definitions | ||
| /// First all local cohorts which are computable from the query data are evaluated, then any remaining local cohorts are set to "otherwise" | ||
| /// Computation starts from such a point, such that dependencies can be resolved in a depth-first manner | ||
| /// | ||
| /// Values of regular and remote cohort dimensions in query_data are retained as is. | ||
| /// Returned value, might have a different value for local cohort dimensions based on its based on dimensions, | ||
| /// if the value provided for the local cohort was incorrect in the query data. | ||
| pub fn evaluate_local_cohorts( | ||
| dimensions: &HashMap<String, DimensionInfo>, | ||
| query_data: &Map<String, Value>, | ||
| ) -> Map<String, Value> { | ||
| _evaluate_local_cohorts(dimensions, query_data, false) | ||
| } | ||
|
|
||
| /// Same as evaluate_local_cohorts but does not set unresolved local cohorts to "otherwise" | ||
| pub fn evaluate_local_cohorts_skip_unresolved( | ||
| dimensions: &HashMap<String, DimensionInfo>, | ||
| query_data: &Map<String, Value>, | ||
| ) -> Map<String, Value> { | ||
| _evaluate_local_cohorts(dimensions, query_data, true) | ||
| } | ||
|
|
||
| /// Identifies starting dimensions for evaluation based on query data and dimension definitions | ||
| /// For each tree in the dependency graph, picks the node closest to root from query_data for each branch of the tree. | ||
| /// If nothing is found and a local cohort is encountered, picks that local cohort as start point from that branch. | ||
|
|
||
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.
Missing null check for
c_dimensionsparameter.Unlike
filter_prefixwhich has explicit null handling,c_dimensionsis passed directly tocstring_to_rstringwithout a null check. If a caller passesNULLfor dimensions, this will cause undefined behavior whenCStr::from_ptris called on a null pointer.Consider adding a null check similar to the
filter_prefixhandling, or document thatc_dimensionsmust never be null.🐛 Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents