feat(promql): constant-fold min_of / max_of scalar reducers (#89) - #96
Merged
Merged
Conversation
`min_of`/`max_of` are n-ary *scalar* reducers (Scalar…→Scalar) that parsed but were rejected. When I split #89 off #51 I expected they'd need a scalar min/max IR node and wouldn't help the corpus — but the corpus splits in two: 14 of the 27 uses are bare constant forms (`min_of(3, 5)`, `max_of(-2, -5)`, NaN cases) in ordinary scalar positions, and only the other 13 are nested with `step()`/`range()` inside dynamic range / offset positions that are themselves unsupported (unrelated to min_of/max_of). So fold the constant case into the existing `Scalar` leaf, the same mechanism as scalar arithmetic (#35): - `num_expr` gains a `min_of`/`max_of` arm that folds when every argument is a constant scalar, reducing with `f64::min`/`max` (which ignore NaN, matching PromQL's `min`/`max` semantics). A non-constant argument (`step()`) fails the recursive fold and propagates the error, so those stay rejected — there is no scalar min/max node and they only occur in unsupported dynamic contexts. - `walk` folds a bare top-level `min_of(consts…)` query to a `Scalar` leaf; operand/nested positions already route through `num_expr` via `scalar_or_vector`. Tests: conformance §U — constant folds (incl. negatives, nesting, threshold operand), NaN-ignoring, and a `__GAP` pinning that non-constant (`step()`) forms stay rejected. Flipped the old blanket-rejection pin. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #89.
What
min_of/max_ofare n-ary scalar reducers (Scalar…→Scalar) that parsed but were rejected.When I split #89 off #51 I assumed they'd need a scalar min/max IR node and that folding "wouldn't help the real corpus cases." Looking closely, that was too pessimistic — the 27 corpus uses split cleanly:
min_of(3, 5),max_of(-2, -5), the NaN cases) in ordinary scalar positions — foldable now;step()/range()inside dynamic-range /offsetpositions — still blocked, but on those separate features, not onmin_of/max_ofitself.How
Fold the constant case into the existing
Scalarleaf — the exact mechanism scalar arithmetic already uses (#35), so no new IR node:num_exprgains amin_of/max_ofarm that folds when every argument is a constant scalar, reducing withf64::min/max(which ignore NaN, matching PromQL'smin/maxsemantics). A non-constant argument (step()) fails the recursive fold and propagates the error, so it stays rejected.walkfolds a bare top-levelmin_of(consts…)query to aScalarleaf; operand and nested positions already route throughnum_exprviascalar_or_vector.Scope
The remaining
min_of(step()+1, 1h)/offset min_of(step(), 1s)corpus forms stay rejected — but that's becausestep()/range()and dynamic range/offsetexpressions are unsupported, which is orthogonal tomin_of/max_of. Those would flip on for free once those features land; no scalar min/max node is needed. Somin_of/max_ofnow lower everywhere they can.Tests
Conformance §U:
min_of(3,5)→3,max_of(3,5)→5, negatives, nesting (max_of(min_of(2,3), 10)→10), and as a threshold operand (up > max_of(1,2));max_of(3, NaN)→3);__GAPpinning that non-constant (step()) forms stay rejected.Flipped the old blanket-rejection pin. Full workspace suite green (corpus ratchet picks up the newly-folding queries); clippy clean.