diff --git a/src/promql/src/histogram_eval/aggregate.rs b/src/promql/src/histogram_eval/aggregate.rs index 5640ea30066..6653c5a40b6 100644 --- a/src/promql/src/histogram_eval/aggregate.rs +++ b/src/promql/src/histogram_eval/aggregate.rs @@ -128,33 +128,31 @@ fn aggregate_point<'a>( if incompatible { continue; } + // The only addition error Go's `handleAggregationError` absorbs; others propagate. + if current.uses_custom_buckets() != h.uses_custom_buckets() { + incompatible = true; + continue; + } match op { - HistogramAggregate::Sum => match current.kahan_add(&h, compensation.take()) { - Ok((c, _)) => compensation = Some(c), - Err(_) => incompatible = true, - }, + HistogramAggregate::Sum => { + let (c, _) = current.kahan_add(&h, compensation.take())?; + compensation = Some(c); + } HistogramAggregate::Avg => { if mean.is_none() { let mut sum = current.clone(); - match sum.kahan_add(&h, compensation.clone()) { - Ok((c, _)) if !sum.has_overflow() => { - *current = sum; - compensation = Some(c); - continue; - } - Ok(_) => { - let mut started = current.clone(); - started.div(count - 1.0); - if let Some(c) = compensation.as_mut() { - c.div(count - 1.0); - } - mean = Some(started); - } - Err(_) => { - incompatible = true; - continue; - } + let (c, _) = sum.kahan_add(&h, compensation.clone())?; + if !sum.has_overflow() { + *current = sum; + compensation = Some(c); + continue; } + let mut started = current.clone(); + started.div(count - 1.0); + if let Some(c) = compensation.as_mut() { + c.div(count - 1.0); + } + mean = Some(started); } // Go switches to an incremental mean once the plain sum overflows. let q = (count - 1.0) / count; @@ -165,10 +163,8 @@ fn aggregate_point<'a>( to_add.div(count); let mean = mean.as_mut().expect("set above"); mean.mul(q); - match mean.kahan_add(&to_add, compensation.take()) { - Ok((c, _)) => compensation = Some(c), - Err(_) => incompatible = true, - } + let (c, _) = mean.kahan_add(&to_add, compensation.take())?; + compensation = Some(c); } } } diff --git a/src/promql/src/histogram_eval/float_histogram.rs b/src/promql/src/histogram_eval/float_histogram.rs index 1f984c4635a..ce10ae67ff8 100644 --- a/src/promql/src/histogram_eval/float_histogram.rs +++ b/src/promql/src/histogram_eval/float_histogram.rs @@ -679,8 +679,6 @@ fn iterate(side: &Buckets, schema: i32, target: i32, threshold: f64) -> Result last.1 += count, - // A merged bucket starts its sum from zero. - _ if schema != target => out.push((target_idx, 0.0 + count)), _ => out.push((target_idx, count)), } } diff --git a/src/promql/src/histogram_eval/tests.rs b/src/promql/src/histogram_eval/tests.rs index 4d2c47c22ff..42640548919 100644 --- a/src/promql/src/histogram_eval/tests.rs +++ b/src/promql/src/histogram_eval/tests.rs @@ -691,3 +691,54 @@ fn widened_zero_threshold_uses_go_bounds() { )); assert!(!current.detect_reset(&previous).unwrap()); } + +/// Only mixing exponential and custom buckets ends a group without a result; any other +/// addition error, such as an invalid layout, fails the aggregation. +#[test] +fn aggregation_propagates_errors_other_than_mixed_schemas() { + let grid = Arc::new( + datatypes::series::time::EvalGrid::try_new( + datatypes::series::time::GridSpec::try_new(0, 1_000, 1).unwrap(), + ) + .unwrap(), + ); + let schema = Arc::new(SeriesSchema { + fields: vec![FieldSpec { + name: Arc::from("value"), + }] + .into(), + eval_grid: Some(grid), + }); + let group = |texts: [&str; 2]| -> Vec { + texts + .iter() + .enumerate() + .map(|(host, text)| { + let labels = Labels::from([("host".to_string(), host.to_string())]); + let histograms = BTreeMap::from([(0, text.to_string())]); + mixed_series(&schema, &labels, &BTreeMap::new(), &histograms).unwrap() + }) + .collect() + }; + let exponential = "0/0/0/3ff0000000000000/3ff0000000000000/0/1_1/3ff0000000000000///"; + let custom = + "-53/0/0/3ff0000000000000/3ff0000000000000/0/0_1/3ff0000000000000///3ff0000000000000"; + let invalid = + "9/3ff0000000000000/0/3ff0000000000000/3ff0000000000000/0/1_1/3ff0000000000000///"; + for op in [HistogramAggregate::Sum, HistogramAggregate::Avg] { + let mixed = aggregate_histograms( + op, + &schema, + &group([exponential, custom]), + metric_of(&Labels::new()), + ); + assert!(mixed.unwrap().is_none()); + let invalid = aggregate_histograms( + op, + &schema, + &group([invalid, invalid]), + metric_of(&Labels::new()), + ); + assert!(invalid.is_err()); + } +}