From c46df87080cf4e70b4c1ba25c99c7ab2ebef7aab Mon Sep 17 00:00:00 2001 From: zz_y Date: Thu, 2 Jul 2026 15:56:36 -0600 Subject: [PATCH] =?UTF-8?q?fix:=20review=20hardening=20=E2=80=94=20count?= =?UTF-8?q?=5Fover=5Ftime=20dtype,=20SampleValue=20binding,=20range=20guar?= =?UTF-8?q?d=20(#69,=20#70,=20#71)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds the three low/hardening findings from the whole-repo review: #69 — count_over_time value column typed Int64 instead of Float64. per_series_reduction_schema now forces the value dtype to Float64: a per-series range reduction always produces a PromQL float sample value, so every reducer (incl. count_over_time, whose Count intent types Int64) matches. + conformance test. #70 — SampleValue could bind a label column. The `SampleValue` fallback tried "the sole non-timestamp column of any type" before "the sole numeric column", so a `[ts, host:Utf8]` schema bound to `host`. Dropped the type-agnostic step; the numeric-only fallback subsumes every legitimate case and now fails cleanly on a label-only schema. + unit test. #71 — counter-derivative could be emitted range-less. If a changes/delta/deriv/resets/idelta/predict_linear/double_exp intent reaches the converter without an enclosing Window (unlike rate/increase it carries no window in its AggFunc), the range would be silently dropped. Added a ConvertError::RangelessRangeReduction guard. + unit test. (Defensive: the front ends always wrap these in a Window today.) No behavior change on valid input; full workspace suite green; clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tests/promql_conformance.rs | 15 +++++ crates/ir/src/intent_algebra/query_expr.rs | 5 ++ crates/l2/src/column_resolution.rs | 42 +++++++++----- crates/l2/src/lower.rs | 55 +++++++++++++++++++ 4 files changed, 102 insertions(+), 15 deletions(-) diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index b0de4929..07a9d0f3 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -33,6 +33,7 @@ use std::time::Duration; +use asap_ir::intent_algebra::schema::DataType; use asap_ir::intent_algebra::{ AggIntent, ArithOp, BinaryOpKind, CompareOp, QueryExpr, Source, }; @@ -763,6 +764,20 @@ fn unsupported_functions_are_rejected() { // M. Counter-derivative range functions (functions.test; issue #44) // ───────────────────────────────────────────────────────────────────────────── +#[test] +fn count_over_time_value_column_is_float64() { + // #69: a per-series range reduction produces a PromQL sample value, which is + // always float64. `count_over_time`'s `Count` intent types `Int64`, but the + // derived `value` column must be `Float64` like every other range reducer. + let schema = ok("count_over_time(m[5m])").output_schema().unwrap(); + let value = schema + .columns + .iter() + .find(|c| c.name == "value") + .expect("value column"); + assert_eq!(value.dtype, DataType::Float64); +} + #[test] fn counter_derivative_functions_lower_to_distinct_intents() { // Each range function reduces one series' window to one value per series diff --git a/crates/ir/src/intent_algebra/query_expr.rs b/crates/ir/src/intent_algebra/query_expr.rs index a5454d1c..4ba46f74 100644 --- a/crates/ir/src/intent_algebra/query_expr.rs +++ b/crates/ir/src/intent_algebra/query_expr.rs @@ -660,6 +660,11 @@ fn per_series_reduction_schema(input: &Schema, agg: &AggIntent) -> Schema { if let Some(vi) = value_idx { let mut out = agg.output_column(&columns[vi]); out.name = "value".into(); + // A per-series range reduction produces a PromQL sample value, which is + // always `float64` — override the reducer's own output dtype so + // `count_over_time` (whose `Count` intent types `Int64`) matches every + // other range reducer instead of leaking an `Int64` value column (#69). + out.dtype = DataType::Float64; columns[vi] = out; } Schema { diff --git a/crates/l2/src/column_resolution.rs b/crates/l2/src/column_resolution.rs index 485e6381..f9c82fa7 100644 --- a/crates/l2/src/column_resolution.rs +++ b/crates/l2/src/column_resolution.rs @@ -69,21 +69,14 @@ pub fn resolve_column_ref(col: &ColumnRef, schema: &Schema) -> Result schema .column_id("value") .or_else(|| { - // After an aggregate the sample value is renamed (e.g. "avg"); - // fall back to the sole non-timestamp column when unambiguous. - let non_ts: Vec = (0..schema.columns.len()) - .filter(|&i| Some(i) != schema.time_index) - .collect(); - (non_ts.len() == 1).then(|| non_ts[0]) - }) - .or_else(|| { - // A *cross-series* aggregate (`sum by (job) (…)`) emits the group - // labels (Utf8) alongside the single numeric value column (e.g. - // `[job:Utf8, sum:Float64]`), so the sole-non-ts fallback above is - // ambiguous. The PromQL sample value of such a vector is that one - // numeric column — the labels are keys, not values. Pick it when - // it is the unique non-timestamp numeric column, so an outer - // ranking (`topk(k, sum by (job) (…))`) resolves its sort key. + // After an aggregate the sample value is renamed (e.g. "avg", or + // "sum" alongside group labels in `[job:Utf8, sum:Float64]`). + // Fall back to the unique non-timestamp *numeric* column — the + // sample value is always numeric, and the labels are keys, not + // values. Requiring numeric (rather than "the sole non-ts column + // of any type") avoids binding `SampleValue` to a label column in + // a `[ts, host:Utf8]`-shaped schema (#70), and still resolves an + // outer ranking's sort key (`topk(k, sum by (job) (…))`). let numeric: Vec = (0..schema.columns.len()) .filter(|&i| Some(i) != schema.time_index) .filter(|&i| { @@ -246,6 +239,25 @@ mod tests { assert_eq!(resolve_column_ref(&ColumnRef::SampleValue, &s), Ok(1)); } + #[test] + fn sample_value_does_not_bind_a_label_column() { + // `[ts:Timestamp, host:Utf8]` has no `value` column and its sole non-ts + // column is a *label* (Utf8), not a sample value. `SampleValue` must not + // bind to it (#70) — resolution fails cleanly instead of picking a label. + let s = Schema::with_time_index( + vec![ + Column::new("ts", DataType::Timestamp, false), + Column::new("host", DataType::Utf8, true), + ], + 0, + vec![], + ); + assert!(matches!( + resolve_column_ref(&ColumnRef::SampleValue, &s), + Err(ResolveError::NoSampleValue { .. }) + )); + } + #[test] fn sample_value_ambiguous_when_two_numeric_columns() { // Two numeric non-ts columns → genuinely ambiguous → NoSampleValue. diff --git a/crates/l2/src/lower.rs b/crates/l2/src/lower.rs index ec32ca6b..062443d4 100644 --- a/crates/l2/src/lower.rs +++ b/crates/l2/src/lower.rs @@ -47,6 +47,13 @@ pub enum ConvertError { #[error("group keys on a per-series windowed reduction are unsupported \ (per-group ranking must use Sort.partition_by — see issue #12)")] WindowedReductionKeys, + /// A counter-derivative range function (`changes`/`delta`/`deriv`/…) reached + /// the converter without an enclosing `Window`, so no range could be + /// recovered. Emitting it range-less would silently drop its window, so this + /// signals a malformed L2 tree rather than a valid instant aggregate (#71). + #[error("counter-derivative range function has no window \ + (its range would be silently dropped)")] + RangelessRangeReduction, } /// Lower a Layer-2 tree to canonical L3, threading `accuracy` onto every @@ -139,6 +146,26 @@ pub fn convert( (other, range) } }; + // A counter-derivative range function is per-series over a range + // and always arrives under an L2 `Window` from the front ends. + // If one reaches here range-less (no `Window`, and unlike + // `Rate`/`Increase` it carries no window in its `AggFunc`), + // emitting it without a `TimeRange` would silently drop its + // window — reject the malformed tree instead (issue #71). + if time_range.is_none() + && matches!( + &aggs[0].func, + AggFunc::Changes + | AggFunc::Delta + | AggFunc::IDelta + | AggFunc::Deriv + | AggFunc::Resets + | AggFunc::PredictLinear { .. } + | AggFunc::DoubleExpSmoothing { .. } + ) + { + return Err(ConvertError::RangelessRangeReduction); + } let agg_child_raw = convert(agg_input_l2, fallback, acc)?; let agg_in_schema = agg_child_raw.output_schema()?; let intent = agg_func_to_intent( @@ -560,6 +587,34 @@ mod tests { /// A SQL-shaped `SELECT SUM(bytes), AVG(latency) FROM t` lowers each /// reducer onto its own input column (positional), and the derived output /// schema types each result off that column (`SUM(bytes:Int64)→Int64`). + #[test] + fn counter_derivative_without_window_is_rejected() { + // A counter-derivative (`Changes`) is per-series over a range and must + // arrive under an L2 `Window`. If it reaches the converter range-less + // (no `Window`, and unlike `Rate`/`Increase` it carries no window in its + // `AggFunc`), emitting it without a `TimeRange` would silently drop its + // window — the malformed tree is rejected instead (#71). + let schema = Schema::with_time_index( + vec![col("ts", DataType::Timestamp), col("value", DataType::Float64)], + 0, + vec![], + ); + let tree = LQueryExpr::Aggregate { + keys: vec![], + aggs: vec![AggItem { + alias: None, + func: AggFunc::Changes, + col: ColumnRef::SampleValue, + }], + having: None, + input: Box::new(LQueryExpr::Source(SourceSpec::new("m"))), // NO Window + }; + assert!(matches!( + convert(&tree, &schema, &AccuracyTarget::Exact), + Err(ConvertError::RangelessRangeReduction) + )); + } + #[test] fn multi_column_aggregate_threads_per_agg_col() { let schema = Schema::with_time_index(