Skip to content

Commit 2bb185e

Browse files
fix(precompute): correctly parse configuration parameters from planner for CMS with heap (#483)
* fix(precompute): correctly parse configuration parameters from planner for CMS with heap * removed unused sub_type
1 parent 3e36cae commit 2bb185e

1 file changed

Lines changed: 56 additions & 15 deletions

File tree

asap-query-engine/src/precompute_engine/accumulator_factory.rs

Lines changed: 56 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -711,19 +711,24 @@ fn kll_k_param(config: &AggregationConfig) -> Result<u16, String> {
711711
}
712712

713713
/// Extract `(row_num, col_num)` for CMS / HydraKLL configs.
714+
///
715+
/// Accepts the planner-canonical `depth`/`width` names first, then falls back
716+
/// to the `row_num`/`col_num` aliases — mirroring `cms_heap_params()`.
714717
fn cms_params(config: &AggregationConfig) -> Result<(usize, usize), String> {
715-
let row_num = config
716-
.parameters
717-
.get("row_num")
718-
.and_then(|v| v.as_u64())
719-
.ok_or_else(|| "CMS config missing required parameter: row_num".to_string())?
720-
as usize;
721-
let col_num = config
722-
.parameters
723-
.get("col_num")
724-
.and_then(|v| v.as_u64())
725-
.ok_or_else(|| "CMS config missing required parameter: col_num".to_string())?
726-
as usize;
718+
let read = |names: &[&str]| -> Result<usize, String> {
719+
names
720+
.iter()
721+
.find_map(|n| config.parameters.get(*n).and_then(|v| v.as_u64()))
722+
.map(|v| v as usize)
723+
.ok_or_else(|| {
724+
format!(
725+
"CMS config missing required parameter (tried: {})",
726+
names.join(", ")
727+
)
728+
})
729+
};
730+
let row_num = read(&["depth", "row_num"])?;
731+
let col_num = read(&["width", "col_num"])?;
727732
Ok((row_num, col_num))
728733
}
729734

@@ -1336,10 +1341,46 @@ mod tests {
13361341
);
13371342
let err = create_accumulator_updater(&config)
13381343
.err()
1339-
.expect("expected Err for missing row_num param");
1344+
.expect("expected Err for missing params");
1345+
// Error must mention both aliases so callers know what's accepted.
13401346
assert!(
1341-
err.contains("row_num"),
1342-
"error should mention the missing parameter name"
1347+
err.contains("depth") && err.contains("row_num"),
1348+
"error should mention accepted parameter names, got: {err}"
1349+
);
1350+
}
1351+
1352+
#[test]
1353+
fn test_cms_depth_width_params_accepted() {
1354+
// Planner emits depth/width; engine must accept them without error.
1355+
use std::collections::HashMap;
1356+
let mut params = HashMap::new();
1357+
params.insert("depth".to_string(), serde_json::json!(3_u64));
1358+
params.insert("width".to_string(), serde_json::json!(1024_u64));
1359+
let config = AggregationConfig::new(
1360+
21,
1361+
AggregationType::CountMinSketch,
1362+
String::new(),
1363+
params,
1364+
promql_utilities::data_model::key_by_label_names::KeyByLabelNames::new(vec![]),
1365+
promql_utilities::data_model::key_by_label_names::KeyByLabelNames::new(vec![]),
1366+
promql_utilities::data_model::key_by_label_names::KeyByLabelNames::new(vec![]),
1367+
String::new(),
1368+
15_000,
1369+
0,
1370+
WindowType::Tumbling,
1371+
"fake_metric".to_string(),
1372+
"fake_metric".to_string(),
1373+
None,
1374+
None,
1375+
None,
1376+
None,
1377+
);
1378+
let updater = create_accumulator_updater(&config)
1379+
.expect("depth/width params must be accepted by CountMinSketch");
1380+
assert!(updater.is_keyed());
1381+
assert_eq!(
1382+
updater.snapshot_accumulator().type_name(),
1383+
"CountMinSketchAccumulator"
13431384
);
13441385
}
13451386

0 commit comments

Comments
 (0)