From 6280f79bcfa77e844e6298d6e316598f95db8415 Mon Sep 17 00:00:00 2001 From: Milind Srivastava Date: Mon, 24 Aug 2026 21:41:03 -0400 Subject: [PATCH] fix(query-engine): skip orphaned dual-population groups in instant queries instead of failing collect_results_separate_keys hard-failed the entire instant query whenever a keys-side group had no matching entry in merged_values, diverging from the range path's #583 fix which skips the orphaned group with a warning instead. Bring instant in line with range (#597). Co-Authored-By: Claude Sonnet 5 --- .../src/engines/simple_engine/mod.rs | 17 +++- .../src/tests/native_binary_instant_tests.rs | 81 +++++++++++++++++++ 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/asap-query-engine/src/engines/simple_engine/mod.rs b/asap-query-engine/src/engines/simple_engine/mod.rs index 410b810c..53c3706e 100644 --- a/asap-query-engine/src/engines/simple_engine/mod.rs +++ b/asap-query-engine/src/engines/simple_engine/mod.rs @@ -1258,11 +1258,20 @@ impl SimpleEngine { .get_keys() .ok_or_else(|| "Keys required for separate aggregation".to_string())?; - for key_for_this_precompute in keys_for_this_precompute { - let value_precompute = merged_values - .get(key) - .ok_or_else(|| format!("No value for key: {:?}", key))?; + // A group with keys data but no matching value data is skipped + // instead of failing the whole query, mirroring the range + // query's #583 behavior (previously `.ok_or_else(...)?` here + // hard-failed everything for one missing group; see #597). + let Some(value_precompute) = merged_values.get(key) else { + warn!( + "Instant query: group {:?} has keys data but no value data -- \ + skipping this group instead of failing the whole query (#597)", + key + ); + continue; + }; + for key_for_this_precompute in keys_for_this_precompute { let value = self .query_precompute_for_statistic( value_precompute.as_ref(), diff --git a/asap-query-engine/src/tests/native_binary_instant_tests.rs b/asap-query-engine/src/tests/native_binary_instant_tests.rs index 4f0bd30c..37469086 100644 --- a/asap-query-engine/src/tests/native_binary_instant_tests.rs +++ b/asap-query-engine/src/tests/native_binary_instant_tests.rs @@ -257,6 +257,87 @@ mod tests { assert!(!sorted(vector_values(qr)).is_empty()); } + #[tokio::test(flavor = "multi_thread")] + async fn instant_query_dual_population_group_with_no_value_data_is_skipped_not_fatal() { + // Instant-query counterpart to + // native_range_query_tests::range_query_dual_population_group_with_no_value_data_is_skipped_not_fatal. + // collect_results_separate_keys used to hard-fail the ENTIRE instant + // query if any group resolved from merged_keys had no matching entry + // in merged_values: `merged_values.get(key).ok_or_else(|| "No value + // for key")?`. region=orphan has keys data (a real DeltaSetAggregator + // key) but never has any value/CMS data at all -- that poisoned the + // WHOLE query, so even region=normal's perfectly good data + // disappeared. Per #597 (bringing the instant path in line with + // #583's range-query fix), this is now skipped with a warning + // instead, and the rest of the query's results still return. + let cms_normal = CountMinSketchAccumulator::new(2, 3); + + let mut keys_normal = DeltaSetAggregatorAccumulator::new(); + keys_normal.add_key(KeyByLabelValues { + labels: vec![ + "normal".to_string(), + "host-a".to_string(), + "evt-1".to_string(), + ], + }); + let mut keys_orphan = DeltaSetAggregatorAccumulator::new(); + keys_orphan.add_key(KeyByLabelValues { + labels: vec![ + "orphan".to_string(), + "host-z".to_string(), + "evt-1".to_string(), + ], + }); + + let engine = create_engine_dual_input( + "event_frequency", + AggregationType::CountMinSketch, + AggregationType::DeltaSetAggregator, + vec!["region"], + vec!["host", "event"], + vec![( + Some(vec!["normal".to_string()]), + Box::new(cms_normal) as Box, + )], + // Deliberately NO value data for region=orphan. + vec![ + ( + Some(vec!["normal".to_string()]), + Box::new(keys_normal) as Box, + ), + ( + Some(vec!["orphan".to_string()]), + Box::new(keys_orphan) as Box, + ), + ], + "count(event_frequency) by (region, host, event)", + ); + + let query = "count(event_frequency) by (region, host, event)"; + let (_, qr) = engine + .handle_query_promql(query.to_string(), QUERY_TIME) + .expect( + "instant query should succeed by skipping the value-less region=orphan \ + group, not fail the entire query because of it", + ); + let values = vector_values(qr); + + assert!( + values + .iter() + .any(|(labels, _)| labels.contains(&"normal".to_string())), + "region=normal has real value data and should be unaffected by \ + region=orphan having none" + ); + assert!( + !values + .iter() + .any(|(labels, _)| labels.contains(&"orphan".to_string())), + "region=orphan has keys data but no value data anywhere -- it must be \ + silently skipped, not appear as an (empty or otherwise) series" + ); + } + #[tokio::test(flavor = "multi_thread")] async fn binary_expr_sliding_window_end_to_end_merges_correctly() { // Ties Stage 1's sliding-bucket merge fix (#570) to the actual