Skip to content

fix: reject PromQL fill modifiers instead of ignoring them - #494

Closed
zzylol wants to merge 2 commits into
feat/promql-comparisons-setsfrom
fix/reject-promql-fill
Closed

zzylol wants to merge 2 commits into
feat/promql-comparisons-setsfrom
fix/reject-promql-fill

Conversation

@zzylol

@zzylol zzylol commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #492.

Rebuilt into the linear stack on main. New integrate: commit(s) fold in integration-branch resolutions this PR needs on top of the earlier stack: "record that PromQL fill modifiers are rejected".

Why

VectorMatch has no fill field, so the PromQL frontend lowered fill, fill_left, and fill_right as if they weren't there. That changes which series are emitted and their values. A lowering error is the correct result: the deployment then sends the query to external exact execution.

What

  • walk_binary returns UnsupportedFeature("fill vector-matching modifier: ...") when a binary modifier has a fill value on either side. The parser puts every fill form into BinModifier::fill_values, so this check covers all of them.
  • Regression test fill_modifiers_are_rejected_not_ignored covers fill, fill_left, fill_right, both orderings of fill_left ... fill_right, and combinations with on/ignoring/group_left/group_right/bool, including one nested in an aggregate.
  • The MetricsQL frontend needs no change. Its vendored parser has no fill keyword, so these queries never reach lowering there.

Before this PR

left_vector + fill(0) right_vector lowered to BinaryOp { vector_match: Some(Ignoring []) }, which is the same tree as left_vector + right_vector.

After this PR

The same query fails to lower with unsupported feature: fill vector-matching modifier: ....

Testdata corpus floor (promql_corpus.rs): 1495 → 1485 (measured lowered count drops from 1529 to 1485). 44 queries are affected, all in promql_corpus_testdata.txt (lines 342–386). Examples:

  • left_vector + fill_left(5) fill_right(7) right_vector
  • left_vector == bool fill(30) right_vector
  • requests / on(status) group_left fill_right(1) limits
  • node_meta * on(instance) group_right fill_left(1) cpu_info

The docs corpus (47), awesome-prometheus-alerts, o11y-bench, and metrics-observability floors are unchanged. None of those corpora contain fill.

Validation

  • The regression test fails before the fix (Ok(BinaryOp ...)) and passes after it.
  • cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace --no-fail-fast all pass (1399 passed, 0 failed).

🤖 Generated with Claude Code

zzylol and others added 2 commits September 30, 2026 18:37
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 <noreply@anthropic.com>
#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 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the feat/promql-comparisons-sets branch from 3fcb9e3 to 50f4aef Compare September 30, 2026 20:24
@zzylol
zzylol force-pushed the fix/reject-promql-fill branch from feade00 to 6eb8a84 Compare September 30, 2026 20:24
zzylol added a commit that referenced this pull request Oct 1, 2026
…togram_quantile (#489, #490, #492, #494, #493)

Per-series vector arithmetic with Prometheus summation, duplicate label-set rejection, comparisons and set operators, rejection of fill modifiers, and histogram_quantile.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol

zzylol commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Included in the squash merge of #493 into main (deb3904).

@zzylol zzylol closed this Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant