Skip to content

feat: compile PromQL comparisons and set operators - #492

Closed
zzylol wants to merge 4 commits into
fix/series-labels-duplicatesfrom
feat/promql-comparisons-sets
Closed

zzylol wants to merge 4 commits into
fix/series-labels-duplicatesfrom
feat/promql-comparisons-sets

Conversation

@zzylol

@zzylol zzylol commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #490.

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: "widen shared series identity to every PromQL binary operator". Conflict resolutions are recorded in the messages of: "feat(physical): compile PromQL comparisons, set operators, and group modifiers".

Why

compile rejected PromQL comparisons, set operators, group_left/group_right, and non-literal scalar operands, so the backend still had to lower them itself. The frontend also dropped bool, so a > bool 1 lowered the same way as the filter a > 1.

What

  • IR: a new variant, BinaryOpKind::CompareBool(CompareOpKind), sits beside the filtering Compare. The PromQL frontend emits it for bool. I used a variant rather than a return_bool flag for two reasons: only comparisons accept bool, and the variant reaches both QueryExpr::BinaryOp and the post-ASAP Binary payload without changing any construction sites.
  • Operator::series_binary now implements Prometheus' VectorBinop, VectorAnd/Or/Unless, and vector-scalar semantics:
    • arithmetic, comparison filters, and bool comparisons
    • vector, literal, scalar(), and scalar-expression operands
    • one-to-one matching, and group_left/group_right with included labels
    • and/or/unless with on/ignoring
    • __name__ and duplicate handling as in Prometheus
  • Where it is used: Fallback subtrees and query-time Binary nodes. This replaces the literal projection, the relabel+binary chain, and the grouped relational join.
  • Range functions: in the Fallback, every range function except last_over_time now drops __name__, and series whose label sets then become equal are an error.
  • Label-map vector_binary: it now treats CompareBool as its bool mode.

Before this PR

a > bool 1                    -> lowered as the filter a > 1
a > 5, a and b, a or vector(0) -> compile error
a * on(job) group_left(team) info, a * scalar(b), sum by (job)(a) * 2 -> compile error
rate({job="x"}[1m]) over series differing only in __name__ -> two series

After this PR

The results below were checked against hand-computed Prometheus results.

  • a = {job=x} 10, {job=y} 20, {job=w} 0, {job=n} NaN:
    • a > 5 gives a{job=x} 10, a{job=y} 20.
    • a > bool 5 gives {job=n} 0, {job=w} 0, {job=x} 1, {job=y} 1.
  • a * on(job) group_left(team) info gives {inst=1,job=x,team=t1} 20, ….
  • sum(a) or vector(0) gives {} 0 when a is empty.
  • rate(...) over series whose label sets are equal without the name fails with "vector cannot contain metrics with the same labelset".

Remaining

  • Per-series Binary filters and set operators fail closed. Stored readouts keep __name__ where the range function drops it.
  • Labels with no column fail closed. A group_left label, or right-side labels from or/group_right, cannot be added to aggregated label-column rows, because the left logical schema has no column for them. Example: sum by (job)(a) * on(job) group_left(team) info.
  • time() operands are not covered.
  • NaN and Inf literals do not survive the program's JSON round trip.
  • Inner functions in a subquery have no per-step check for equal label sets.
  • fill modifiers are still ignored by the frontend. Rejecting them drops 27 corpus queries below the lowering floor.
  • The other shapes are listed in docs/develop_docs/physical-compile-coverage.md.

Validation

  • cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace --no-fail-fast all pass.

  • The new Fallback tests fail on the base branch's sources and pass here. The new IR tests do not compile on the base branch.

  • An independent reviewer agent found three bugs, each now covered by a regression test:

    • scalar-valued expression operands such as a + -scalar(b)
    • bool in vector_binary kept __name__
    • the inner subquery function kept __name__

    The reviewer also found that or vector(0) was rejected; that is fixed too.

🤖 Generated with Claude Code

zzylol added a commit that referenced this pull request Sep 30, 2026
…ping

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the fix/series-labels-duplicates branch from 17d1c12 to ad45318 Compare September 30, 2026 18:27
zzylol added a commit that referenced this pull request Sep 30, 2026
…modifiers

series_binary now implements Prometheus' VectorBinop, VectorAnd/Or/Unless
and vector-scalar semantics for Fallback subtrees and query-time Binary
nodes, including scalar() operands. Range functions drop __name__ in the
Fallback and reject equal label sets.

Conflicts with earlier stack changes resolved to the integration tree:
- crates/asap-physical-operators/src/physical_planner/promql_rows.rs: a9b8fdd Merge #492 comparison and set coverage with shared series identity typing

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the feat/promql-comparisons-sets branch from 90c1713 to 3fcb9e3 Compare September 30, 2026 18:27
zzylol added a commit that referenced this pull request Sep 30, 2026
#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 added a commit that referenced this pull request Sep 30, 2026
Conflicts with earlier stack changes resolved to the integration tree:
- docs/develop_docs/physical-compile-coverage.md: 2bea2f6 Merge #493
  classic histogram quantile coverage, without the paragraph that 6d895b7
  in this PR adds later. The comparison section from #492 stays, the
  Remaining list drops the entries both PRs now cover, and the totals
  count both PRs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Sep 30, 2026
This PR's histogram_quantile branch reads a function-level `left_layout`,
but #492 restructured `series_labels::execute` to bind it inside each
binary branch. Bind it from the operator input in the histogram branch,
and format the widened `Kind` match arm. Taken from integration commit
0d132b9.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol and others added 4 commits September 30, 2026 18:37
The frontend dropped the bool modifier, so `a > bool 1` lowered like the
filter `a > 1`. A separate variant keeps bool off non-comparisons and
reaches both the query-level BinaryOp and the post-ASAP Binary payload.

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

series_binary now implements Prometheus' VectorBinop, VectorAnd/Or/Unless
and vector-scalar semantics for Fallback subtrees and query-time Binary
nodes, including scalar() operands. Range functions drop __name__ in the
Fallback and reject equal label sets.

Conflicts with earlier stack changes resolved to the integration tree:
- crates/asap-physical-operators/src/physical_planner/promql_rows.rs: a9b8fdd Merge #492 comparison and set coverage with shared series identity typing

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The comparisons, set operators, and group modifiers compiled here share
series identity through asap-types (moved there by #477), while
promql_rows.rs no longer keeps its own copy. Accept every BinaryOp kind
and group modifier in `with_promql_series_identity`, excluding only
operands that read the evaluation timestamp. Taken from integration
merge a9b8fdd.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the fix/series-labels-duplicates branch from ad45318 to d0f5f97 Compare September 30, 2026 20:24
@zzylol
zzylol force-pushed the feat/promql-comparisons-sets branch from 3fcb9e3 to 50f4aef Compare September 30, 2026 20:24
zzylol added a commit that referenced this pull request Sep 30, 2026
#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 added a commit that referenced this pull request Sep 30, 2026
Conflicts with earlier stack changes resolved to the integration tree:
- docs/develop_docs/physical-compile-coverage.md: 2bea2f6 Merge #493
  classic histogram quantile coverage, without the paragraph that 6d895b7
  in this PR adds later. The comparison section from #492 stays, the
  Remaining list drops the entries both PRs now cover, and the totals
  count both PRs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Sep 30, 2026
This PR's histogram_quantile branch reads a function-level `left_layout`,
but #492 restructured `series_labels::execute` to bind it inside each
binary branch. Bind it from the operator input in the histogram branch,
and format the widened `Kind` match arm. Taken from integration commit
0d132b9.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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