Skip to content

Commit 6280f79

Browse files
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 <noreply@anthropic.com>
1 parent 47daa6c commit 6280f79

2 files changed

Lines changed: 94 additions & 4 deletions

File tree

‎asap-query-engine/src/engines/simple_engine/mod.rs‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1258,11 +1258,20 @@ impl SimpleEngine {
12581258
.get_keys()
12591259
.ok_or_else(|| "Keys required for separate aggregation".to_string())?;
12601260

1261-
for key_for_this_precompute in keys_for_this_precompute {
1262-
let value_precompute = merged_values
1263-
.get(key)
1264-
.ok_or_else(|| format!("No value for key: {:?}", key))?;
1261+
// A group with keys data but no matching value data is skipped
1262+
// instead of failing the whole query, mirroring the range
1263+
// query's #583 behavior (previously `.ok_or_else(...)?` here
1264+
// hard-failed everything for one missing group; see #597).
1265+
let Some(value_precompute) = merged_values.get(key) else {
1266+
warn!(
1267+
"Instant query: group {:?} has keys data but no value data -- \
1268+
skipping this group instead of failing the whole query (#597)",
1269+
key
1270+
);
1271+
continue;
1272+
};
12651273

1274+
for key_for_this_precompute in keys_for_this_precompute {
12661275
let value = self
12671276
.query_precompute_for_statistic(
12681277
value_precompute.as_ref(),

‎asap-query-engine/src/tests/native_binary_instant_tests.rs‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,87 @@ mod tests {
257257
assert!(!sorted(vector_values(qr)).is_empty());
258258
}
259259

260+
#[tokio::test(flavor = "multi_thread")]
261+
async fn instant_query_dual_population_group_with_no_value_data_is_skipped_not_fatal() {
262+
// Instant-query counterpart to
263+
// native_range_query_tests::range_query_dual_population_group_with_no_value_data_is_skipped_not_fatal.
264+
// collect_results_separate_keys used to hard-fail the ENTIRE instant
265+
// query if any group resolved from merged_keys had no matching entry
266+
// in merged_values: `merged_values.get(key).ok_or_else(|| "No value
267+
// for key")?`. region=orphan has keys data (a real DeltaSetAggregator
268+
// key) but never has any value/CMS data at all -- that poisoned the
269+
// WHOLE query, so even region=normal's perfectly good data
270+
// disappeared. Per #597 (bringing the instant path in line with
271+
// #583's range-query fix), this is now skipped with a warning
272+
// instead, and the rest of the query's results still return.
273+
let cms_normal = CountMinSketchAccumulator::new(2, 3);
274+
275+
let mut keys_normal = DeltaSetAggregatorAccumulator::new();
276+
keys_normal.add_key(KeyByLabelValues {
277+
labels: vec![
278+
"normal".to_string(),
279+
"host-a".to_string(),
280+
"evt-1".to_string(),
281+
],
282+
});
283+
let mut keys_orphan = DeltaSetAggregatorAccumulator::new();
284+
keys_orphan.add_key(KeyByLabelValues {
285+
labels: vec![
286+
"orphan".to_string(),
287+
"host-z".to_string(),
288+
"evt-1".to_string(),
289+
],
290+
});
291+
292+
let engine = create_engine_dual_input(
293+
"event_frequency",
294+
AggregationType::CountMinSketch,
295+
AggregationType::DeltaSetAggregator,
296+
vec!["region"],
297+
vec!["host", "event"],
298+
vec![(
299+
Some(vec!["normal".to_string()]),
300+
Box::new(cms_normal) as Box<dyn AggregateCore>,
301+
)],
302+
// Deliberately NO value data for region=orphan.
303+
vec![
304+
(
305+
Some(vec!["normal".to_string()]),
306+
Box::new(keys_normal) as Box<dyn AggregateCore>,
307+
),
308+
(
309+
Some(vec!["orphan".to_string()]),
310+
Box::new(keys_orphan) as Box<dyn AggregateCore>,
311+
),
312+
],
313+
"count(event_frequency) by (region, host, event)",
314+
);
315+
316+
let query = "count(event_frequency) by (region, host, event)";
317+
let (_, qr) = engine
318+
.handle_query_promql(query.to_string(), QUERY_TIME)
319+
.expect(
320+
"instant query should succeed by skipping the value-less region=orphan \
321+
group, not fail the entire query because of it",
322+
);
323+
let values = vector_values(qr);
324+
325+
assert!(
326+
values
327+
.iter()
328+
.any(|(labels, _)| labels.contains(&"normal".to_string())),
329+
"region=normal has real value data and should be unaffected by \
330+
region=orphan having none"
331+
);
332+
assert!(
333+
!values
334+
.iter()
335+
.any(|(labels, _)| labels.contains(&"orphan".to_string())),
336+
"region=orphan has keys data but no value data anywhere -- it must be \
337+
silently skipped, not appear as an (empty or otherwise) series"
338+
);
339+
}
340+
260341
#[tokio::test(flavor = "multi_thread")]
261342
async fn binary_expr_sliding_window_end_to_end_merges_correctly() {
262343
// Ties Stage 1's sliding-bucket merge fix (#570) to the actual

0 commit comments

Comments
 (0)