Summary
Follow-up to #220, scoped out of the PR that resolved Instance 1 (Scalar/Literal, merged as of that PR — PromqlScalar(f64) collapsed into PromqlScalarBridge(Rc<QueryExpr<C>>) wrapping Literal(ScalarValue::Float64(_))).
Instance 2 from #220 remains open:
QueryExpr::BinaryOp { op: BinaryOpKind, lhs, rhs, vector_match } — the relational/vector-tree binary operator node (PromQL binary ops between two vector-shaped QueryExprs, or a vector and a scalar bridge).QueryExpr::Compare { left, op: CompareOpKind, right } and QueryExpr::Arithmetic { op: ArithmeticOpKind, left, right } — the scalar-expression sub-language's own binary comparison/arithmetic nodes.
BinaryOpKind already just wraps ArithmeticOpKind/CompareOpKind (see its doc comment in query_expr.rs), so the same operator vocabulary is expressed through two different node shapes depending on which side of the relational/scalar split the expression sits on — the same duplication pattern Instance 1 had, one level up.
Why this was scoped out of the #220 PR
BinaryOp has real additional data Compare/Arithmetic don't: vector_match: Option<VectorMatch> (PromQL vector-matching semantics — on/ignoring/group_left/group_right), which only makes sense at the relational/vector-tree position. Merging cleanly means either:
Compare/Arithmetic gain an optional vector-match field used only when the node sits at the relational-tree position, orBinaryOp becomes a thin relational wrapper around a Compare/Arithmetic node (mirroring the PromqlScalarBridge wrapper pattern Instance 1 introduced for the Scalar/Literal split), or- some hybrid.
BinaryOp also has a much larger blast radius than PromqlScalar did — roughly 60 call sites across query_expr.rs, canonicalize.rs, resolve.rs, binder.rs, cse.rs, dag_export.rs, both front ends, and every test suite that pattern-matches on it, several with PromQL-specific vector-match assertions that would need care to preserve exactly. That's large enough to want its own dedicated PR rather than folding into the Instance-1 change.
Suggested approach
Given the wrapper pattern that worked for Instance 1, the BinaryOp-as-thin-relational-wrapper direction seems most consistent: keep BinaryOp { op, lhs, rhs, vector_match } at the operator-tree position (it already carries data Compare/Arithmetic don't need), and where op is BinaryOpKind::Arithmetic/BinaryOpKind::Compare, consider whether the operator enum itself (not just the operand types) can be unified with ArithmeticOpKind/CompareOpKind directly rather than wrapped — worth scoping precisely before starting, including whether vector_match-carrying variants (And/Or/Unless/Pow/Atan2, the PromQL-only members of BinaryOpKind with no scalar-IR counterpart) block a full collapse or just need to stay as BinaryOp-only cases.
References
🤖 Generated with Claude Code
Summary
Follow-up to #220, scoped out of the PR that resolved Instance 1 (
Scalar/Literal, merged as of that PR —PromqlScalar(f64)collapsed intoPromqlScalarBridge(Rc<QueryExpr<C>>)wrappingLiteral(ScalarValue::Float64(_))).Instance 2 from #220 remains open:
QueryExpr::BinaryOp { op: BinaryOpKind, lhs, rhs, vector_match }— the relational/vector-tree binary operator node (PromQL binary ops between two vector-shapedQueryExprs, or a vector and a scalar bridge).QueryExpr::Compare { left, op: CompareOpKind, right }andQueryExpr::Arithmetic { op: ArithmeticOpKind, left, right }— the scalar-expression sub-language's own binary comparison/arithmetic nodes.BinaryOpKindalready just wrapsArithmeticOpKind/CompareOpKind(see its doc comment inquery_expr.rs), so the same operator vocabulary is expressed through two different node shapes depending on which side of the relational/scalar split the expression sits on — the same duplication pattern Instance 1 had, one level up.Why this was scoped out of the #220 PR
BinaryOphas real additional dataCompare/Arithmeticdon't:vector_match: Option<VectorMatch>(PromQL vector-matching semantics —on/ignoring/group_left/group_right), which only makes sense at the relational/vector-tree position. Merging cleanly means either:Compare/Arithmeticgain an optional vector-match field used only when the node sits at the relational-tree position, orBinaryOpbecomes a thin relational wrapper around aCompare/Arithmeticnode (mirroring thePromqlScalarBridgewrapper pattern Instance 1 introduced for theScalar/Literalsplit), orBinaryOpalso has a much larger blast radius thanPromqlScalardid — roughly 60 call sites acrossquery_expr.rs,canonicalize.rs,resolve.rs,binder.rs,cse.rs,dag_export.rs, both front ends, and every test suite that pattern-matches on it, several with PromQL-specific vector-match assertions that would need care to preserve exactly. That's large enough to want its own dedicated PR rather than folding into the Instance-1 change.Suggested approach
Given the wrapper pattern that worked for Instance 1, the
BinaryOp-as-thin-relational-wrapper direction seems most consistent: keepBinaryOp { op, lhs, rhs, vector_match }at the operator-tree position (it already carries dataCompare/Arithmeticdon't need), and whereopisBinaryOpKind::Arithmetic/BinaryOpKind::Compare, consider whether the operator enum itself (not just the operand types) can be unified withArithmeticOpKind/CompareOpKinddirectly rather than wrapped — worth scoping precisely before starting, including whethervector_match-carrying variants (And/Or/Unless/Pow/Atan2, the PromQL-only members ofBinaryOpKindwith no scalar-IR counterpart) block a full collapse or just need to stay asBinaryOp-only cases.References
Expr<C>scalar tree intoQueryExpr, which is what created both instances of duplication)🤖 Generated with Claude Code