mirror of
https://github.com/GreptimeTeam/greptimedb.git
synced 2026-09-22 13:15:34 +00:00
fix(promql): keep other fields when one has no sample in the window
Review follow-up. Two things surfaced once range functions started returning NULL for a window without samples. The filter after a function call required every field column to be non-NULL, so on a multi-field table one field with no samples in a window would drop the other fields' results with it. It now keeps a row when any field has a sample, which is the shape a selector already emits. On a single field column the two predicates are identical. `quantile_over_time` returned NaN rather than NULL for a window without samples, so the row survived that filter. Prometheus returns an empty vector there, so the emptiness check now sits in the range UDF; the shared quantile kernel still yields NaN for an empty slice, matching upstream's `quantile()` helper that `quantile_aggr` depends on. Signed-off-by: Dennis Zhuang <killme2008@gmail.com>
This commit is contained in:
@@ -132,7 +132,7 @@ impl QuantileOverTime {
|
||||
value_len,
|
||||
&mut samples,
|
||||
);
|
||||
match quantile_with_scratch(window, quantile, &mut scratch) {
|
||||
match window_quantile(window, quantile, &mut scratch) {
|
||||
Some(value) => result_builder.append_value(value),
|
||||
None => result_builder.append_null(),
|
||||
}
|
||||
@@ -183,7 +183,7 @@ impl QuantileOverTime {
|
||||
value_len,
|
||||
&mut samples,
|
||||
);
|
||||
match quantile_with_scratch(window, quantile, &mut scratch) {
|
||||
match window_quantile(window, quantile, &mut scratch) {
|
||||
Some(value) => result_builder.append_value(value),
|
||||
None => result_builder.append_null(),
|
||||
}
|
||||
@@ -218,6 +218,18 @@ fn window_samples<'a>(
|
||||
samples
|
||||
}
|
||||
|
||||
/// Quantile of one range window, or `None` when the window holds no sample.
|
||||
///
|
||||
/// Prometheus returns an empty vector for a range without float samples rather than the NaN
|
||||
/// that [`quantile_impl`] yields for an empty slice, so the emptiness check belongs here and
|
||||
/// not in the shared kernel.
|
||||
fn window_quantile(values: &[f64], quantile: f64, scratch: &mut Vec<f64>) -> Option<f64> {
|
||||
if values.is_empty() {
|
||||
return None;
|
||||
}
|
||||
quantile_with_scratch(values, quantile, scratch)
|
||||
}
|
||||
|
||||
/// Refer to <https://github.com/prometheus/prometheus/blob/6e2905a4d4ff9b47b1f6d201333f5bd53633f921/promql/quantile.go#L357-L386>
|
||||
pub(crate) fn quantile_impl(values: &[f64], quantile: f64) -> Option<f64> {
|
||||
let mut scratch = Vec::new();
|
||||
@@ -337,8 +349,34 @@ mod tests {
|
||||
let output = extract_array(&QuantileOverTime::quantile_over_time(&input).unwrap()).unwrap();
|
||||
let output = output.as_any().downcast_ref::<Float64Array>().unwrap();
|
||||
|
||||
assert_eq!(output.value(0), 2.5);
|
||||
assert!(output.value(1).is_nan());
|
||||
assert!(output.value(2).is_nan());
|
||||
assert_eq!(
|
||||
output.iter().collect::<Vec<_>>(),
|
||||
vec![Some(2.5), None, None]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn quantile_over_time_keeps_nan_for_an_invalid_quantile() {
|
||||
let ts_array = Arc::new(TimestampMillisecondArray::from_iter_values([0i64, 1000]));
|
||||
let values_array = Arc::new(Float64Array::from_iter_values([1.0, 4.0]));
|
||||
let ranges = [(0, 2)];
|
||||
|
||||
let input = vec![
|
||||
ColumnarValue::Array(Arc::new(
|
||||
RangeArray::from_ranges(ts_array, ranges)
|
||||
.unwrap()
|
||||
.into_dict(),
|
||||
)),
|
||||
ColumnarValue::Array(Arc::new(
|
||||
RangeArray::from_ranges(values_array, ranges)
|
||||
.unwrap()
|
||||
.into_dict(),
|
||||
)),
|
||||
ColumnarValue::Scalar(ScalarValue::Float64(None)),
|
||||
];
|
||||
let output = extract_array(&QuantileOverTime::quantile_over_time(&input).unwrap()).unwrap();
|
||||
let output = output.as_any().downcast_ref::<Float64Array>().unwrap();
|
||||
|
||||
assert!(output.value(0).is_nan());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2142,8 +2142,6 @@ impl PromPlanner {
|
||||
),
|
||||
})
|
||||
};
|
||||
let preserve_any_value =
|
||||
Self::field_columns_are_alternative_samples(input.schema(), &self.ctx.field_columns);
|
||||
let (mut func_exprs, new_tags) = self.create_function_expr(
|
||||
func,
|
||||
args.literals.clone(),
|
||||
@@ -2158,10 +2156,14 @@ impl PromPlanner {
|
||||
func_exprs.push(tsid_col);
|
||||
}
|
||||
|
||||
// Each field column is an independent series, so a row survives as long as any field
|
||||
// produced a sample and the others stay NULL, matching what a selector emits. Requiring
|
||||
// every field to be non-NULL would drop one field's samples because another field has
|
||||
// none in that window.
|
||||
let builder = LogicalPlanBuilder::from(input)
|
||||
.project(func_exprs)
|
||||
.context(DataFusionPlanningSnafu)?
|
||||
.filter(self.create_empty_values_filter_expr(preserve_any_value)?)
|
||||
.filter(self.create_empty_values_filter_expr(true)?)
|
||||
.context(DataFusionPlanningSnafu)?;
|
||||
|
||||
let builder = match func.name {
|
||||
|
||||
@@ -150,6 +150,12 @@ TQL EVAL (1, 1, '1s') changes(sparse_samples[1s]);
|
||||
++
|
||||
++
|
||||
|
||||
-- Prometheus returns an empty vector for a range without samples, not NaN.
|
||||
TQL EVAL (1, 1, '1s') quantile_over_time(0.5, sparse_samples[1s]);
|
||||
|
||||
++
|
||||
++
|
||||
|
||||
DROP TABLE sparse_samples;
|
||||
|
||||
Affected Rows: 0
|
||||
@@ -182,6 +188,26 @@ TQL EVAL (3, 3, '1s') rate(multi_field[4s]);
|
||||
| 1970-01-01T00:00:03 | 1.0 | 10.0 | a |
|
||||
+---------------------+---------------------------------------+---------------------------------------+------+
|
||||
|
||||
-- f1 has two samples in this window and f2 only one, so f1 keeps its result while f2 is NULL.
|
||||
-- SQLNESS SORT_RESULT 3 1
|
||||
TQL EVAL (1, 1, '1s') rate(multi_field[4s]);
|
||||
|
||||
+---------------------+---------------------------------------+---------------------------------------+------+
|
||||
| ts | prom_rate(ts_range,f1,ts,Int64(4000)) | prom_rate(ts_range,f2,ts,Int64(4000)) | host |
|
||||
+---------------------+---------------------------------------+---------------------------------------+------+
|
||||
| 1970-01-01T00:00:01 | 0.5 | | a |
|
||||
+---------------------+---------------------------------------+---------------------------------------+------+
|
||||
|
||||
-- A selector emits the same shape: the row stays, the field without a sample is NULL.
|
||||
-- SQLNESS SORT_RESULT 3 1
|
||||
TQL EVAL (1, 1, '1s') multi_field;
|
||||
|
||||
+---------------------+------+-----+----+
|
||||
| ts | host | f1 | f2 |
|
||||
+---------------------+------+-----+----+
|
||||
| 1970-01-01T00:00:01 | a | 2.0 | |
|
||||
+---------------------+------+-----+----+
|
||||
|
||||
DROP TABLE multi_field;
|
||||
|
||||
Affected Rows: 0
|
||||
|
||||
@@ -59,6 +59,9 @@ TQL EVAL (1, 1, '1s') rate(sparse_samples[1s]);
|
||||
|
||||
TQL EVAL (1, 1, '1s') changes(sparse_samples[1s]);
|
||||
|
||||
-- Prometheus returns an empty vector for a range without samples, not NaN.
|
||||
TQL EVAL (1, 1, '1s') quantile_over_time(0.5, sparse_samples[1s]);
|
||||
|
||||
DROP TABLE sparse_samples;
|
||||
|
||||
CREATE TABLE multi_field (
|
||||
@@ -79,4 +82,12 @@ INSERT INTO multi_field VALUES
|
||||
-- SQLNESS SORT_RESULT 3 1
|
||||
TQL EVAL (3, 3, '1s') rate(multi_field[4s]);
|
||||
|
||||
-- f1 has two samples in this window and f2 only one, so f1 keeps its result while f2 is NULL.
|
||||
-- SQLNESS SORT_RESULT 3 1
|
||||
TQL EVAL (1, 1, '1s') rate(multi_field[4s]);
|
||||
|
||||
-- A selector emits the same shape: the row stays, the field without a sample is NULL.
|
||||
-- SQLNESS SORT_RESULT 3 1
|
||||
TQL EVAL (1, 1, '1s') multi_field;
|
||||
|
||||
DROP TABLE multi_field;
|
||||
|
||||
Reference in New Issue
Block a user