From 9d612edcc63f820a0ac8656cde31a439631ca662 Mon Sep 17 00:00:00 2001 From: zzylol Date: Wed, 30 Sep 2026 13:15:58 +0000 Subject: [PATCH 1/2] fix: reject PromQL fill modifiers instead of ignoring them VectorMatch has no fill field, so lowering silently dropped fill, fill_left, and fill_right, changing query results. Reject them with a fill-specific UnsupportedFeature error so the query falls back to exact execution, and lower the testdata corpus floor to the measured 1485. Co-Authored-By: Claude Opus 5.5 --- crates/frontend-promql/src/promql.rs | 10 +++++++++ .../tests/observability/promql_corpus.rs | 7 ++++-- .../frontend-promql/tests/promql_lowering.rs | 22 +++++++++++++++++++ 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/crates/frontend-promql/src/promql.rs b/crates/frontend-promql/src/promql.rs index 6564003dd..a7d83e763 100644 --- a/crates/frontend-promql/src/promql.rs +++ b/crates/frontend-promql/src/promql.rs @@ -1274,6 +1274,16 @@ fn selector_is_bucket(vs: &VectorSelector) -> bool { fn walk_binary(bin: &BinaryExpr) -> Result { let lhs = scalar_or_vector(&bin.lhs)?; let rhs = scalar_or_vector(&bin.rhs)?; + // `VectorMatch` has no fill field; dropping fill would change which series + // are emitted and their values, so the query must fall back to exact + // execution instead. + if let Some(m) = &bin.modifier { + if m.fill_values.lhs.is_some() || m.fill_values.rhs.is_some() { + return Err(LoweringError::UnsupportedFeature(format!( + "`fill` vector-matching modifier: `{bin}`" + ))); + } + } let op = match (binop(bin.op.id())?, bin.return_bool()) { (BinaryOpKind::Compare(op), true) => BinaryOpKind::CompareBool(op), (op, _) => op, diff --git a/crates/frontend-promql/tests/observability/promql_corpus.rs b/crates/frontend-promql/tests/observability/promql_corpus.rs index beb16d612..f265edb45 100644 --- a/crates/frontend-promql/tests/observability/promql_corpus.rs +++ b/crates/frontend-promql/tests/observability/promql_corpus.rs @@ -155,13 +155,16 @@ fn lowering_is_total_over_the_entire_corpus() { // rather than pin an exact count — ratchet them up as coverage lands. // // The 235 unparseable are parser-fork gaps (issue #108); the rejections are - // lowering gaps (#109). Both shrink over time, so these floors only ever rise. + // lowering gaps (#109). Both shrink over time, so these floors normally only + // rise. Exception: the testdata floor was lowered to the measured 1485 when + // the 44 `fill` vector-matching queries became rejected rather than + // silently lowered without their fill semantics. assert!( docs.lowered >= 47, "docs lowering coverage regressed: {docs:?}" ); assert!( - td.lowered >= 1495, + td.lowered >= 1485, "testdata lowering coverage regressed: {td:?}" ); } diff --git a/crates/frontend-promql/tests/promql_lowering.rs b/crates/frontend-promql/tests/promql_lowering.rs index a71cb5ba7..eea388735 100644 --- a/crates/frontend-promql/tests/promql_lowering.rs +++ b/crates/frontend-promql/tests/promql_lowering.rs @@ -864,6 +864,28 @@ fn pathologically_nested_query_is_rejected_not_stack_overflow() { assert!(format!("{err}").contains("nesting"), "got {err}"); } +// Behavior: every parser-accepted `fill` modifier form is rejected with a +// fill-specific lowering error rather than silently dropped. +#[test] +fn fill_modifiers_are_rejected_not_ignored() { + for q in [ + "a + fill(0) b", + "a + fill_left(1) b", + "a + fill_right(2) b", + "a + fill_left(1) fill_right(2) b", + "a + fill_right(2) fill_left(1) b", + "a + on(job) fill(0) b", + "a * ignoring(instance) group_left(env) fill_right(0) b", + "a > bool on(job) fill(0) b", + "sum(a - on(job) group_right fill_left(0) b)", + ] { + match lower_promql(q, AccuracyTarget::Exact) { + Err(LoweringError::UnsupportedFeature(m)) if m.contains("`fill`") => {} + other => panic!("expected fill rejection for {q:?}, got {other:?}"), + } + } +} + // ── accuracy propagation ────────────────────────────────────────────────────── #[test] From 6eb8a84f3955654a8144dd9fefa4e6f16be64c6e Mon Sep 17 00:00:00 2001 From: zzylol <50204836+zzylol@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:14:16 +0000 Subject: [PATCH 2/2] integrate: record that PromQL fill modifiers are rejected #492 listed `fill`, `fill_left`, and `fill_right` as modifiers the frontend still ignores; this PR rejects them. Taken from integration commit 0d132b9. Co-Authored-By: Claude Opus 5.5 --- docs/develop_docs/physical-compile-coverage.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/develop_docs/physical-compile-coverage.md b/docs/develop_docs/physical-compile-coverage.md index 4d6dc44c7..32f93292f 100644 --- a/docs/develop_docs/physical-compile-coverage.md +++ b/docs/develop_docs/physical-compile-coverage.md @@ -229,6 +229,6 @@ In order of backend usage: aggregated (label-column) rows. The logical output schema, which is the left side's, has no column for them. 9. An equal-label-set check for inner subquery functions, per step. -10. `fill`, `fill_left`, and `fill_right` matching modifiers. The frontend - still ignores them. Rejecting them drops 27 corpus queries below the - lowering floor, so that change needs its own decision. + +`fill`, `fill_left`, and `fill_right` matching modifiers are rejected by the +frontend (#494); they are never silently ignored.