refactor(types): collapse QueryExpr::PromqlScalar into Literal (#220)

[zzylol]

Summary

Resolves Instance 1 of #220 (Scalar(f64) vs Literal(ScalarValue) duplication). Instance 2 (BinaryOp vs Compare/Arithmetic) is scoped out — see below.

What #220 observed

#205 folded the old, separate Expr<C> scalar-expression tree into QueryExpr itself, and in doing so left two node shapes representing the same relational-vs-scalar-sub-language split for what's semantically the same value:

• QueryExpr::Scalar(f64) (renamed PromqlScalar by Consider mapping SQL now()/CURRENT_TIMESTAMP (incl. ClickHouse now()) to EvalTime #184, merged in this PR) — a PromQL number literal / folded constant, appearing only as a BinaryOp operand or a bare-scalar query root.
• QueryExpr::Literal(ScalarValue) — a typed scalar constant used inside the scalar-expression sub-language (Compare/Arithmetic/Case/InList operands, etc.).

Every PromQL scalar is f64, so Literal(ScalarValue::Float64(v)) held exactly the value PromqlScalar(v) did — the two variants existed side by side purely because of which tree position each was allowed to appear in, not because they carried different data.

The fix

Removed PromqlScalar(f64). Added a new marker variant instead:

PromqlScalarBridge(Rc<QueryExpr<C>>)

This is the "wrapper/marker at the bridge points instead of on the leaf itself" option #220 called out. Every construction site that used to write QueryExpr::PromqlScalar(v) now writes QueryExpr::promql_scalar(v), which builds PromqlScalarBridge(Rc::new(Literal(ScalarValue::Float64(v)))) — so the value now always lives in the one Literal variant, and PromqlScalarBridge carries only the fact that this particular Literal sits at an operator-tree position (a BinaryOp operand, a PromqlVectorFromScalar child, or a whole query's scalar-typed root) rather than in a scalar-sub-language position (Compare/Arithmetic/Case/… operand).

How the row-schema-position distinction is now tracked

Previously, output_schema(), canonicalize.rs's children_mut, and resolve.rs's match all used "is this node tagged PromqlScalar or Literal" to tell "this operand has its own row schema" from "this is a nested scalar leaf with none" (QueryExprError::ScalarHasNoRowSchema). Now that both cases can hold the identical Literal(ScalarValue::Float64(_)) node, that split rides on the wrapper, not the variant tag:

• output_schema() has a dedicated arm for PromqlScalarBridge(_) (same "single value column, Float64, closed" schema PromqlScalar used to return) and no longer special-cases Literal at all — a bare Literal still falls through to the scalar-leaf catch-all and still returns Err(ScalarHasNoRowSchema), exactly as before.
• canonicalize.rs's children_mut and cse.rs's rebuild_children treat PromqlScalarBridge as a single opaque unit (no recursion into its child) — the same treatment PromqlScalar got as a leaf, now applied to the wrapper instead.
• resolve.rs routes the bridge's child through resolve_expr (the scalar-sub-language resolver), the same function every other scalar position already uses, rather than the old no-op PromqlScalar(v) => PromqlScalar(*v) passthrough. In practice the child is always a Literal (no ColumnRef to resolve), so this is behavior-preserving, but it's now structurally consistent with how every other scalar position resolves instead of being a special case.
• binder.rs's collect_referenced_columns similarly now peels column refs off the bridge's child via the same named helper every other scalar-typed field uses (a no-op today, since the child is always a Literal, but no longer hand-waved as "never has columns" by construction).

Added QueryExpr::promql_scalar(v: f64) -> Self (constructor) and QueryExpr::as_promql_scalar(&self) -> Option<f64> (accessor, Some only for a bridge wrapping a plain Literal(Float64)) to keep call sites — construction and pattern-matching alike — as close to the old one-variant ergonomics as possible.

Call sites updated

Everywhere that constructed or matched on QueryExpr::PromqlScalar:

• crates/types/src/pre_asap/{query_expr.rs, canonicalize.rs, resolve.rs, binder.rs, cse.rs, dag_export.rs}
• crates/types/src/dag_export.rs and crates/types/src/pre_asap/cse.rs — both touched by the currently in-flight CSE work (New feature: add common sub expr elimination #212/Design: pre-ASAP structural CSE via bottom-up hash-consing over Rc<QueryExpr> #223/CSE: decide rule-based vs. cost-based framework for whether to actually share a detected common subexpression #237) in other worktrees; kept to the minimal surgical edit needed to keep them compiling and behaviorally identical (no structural changes to either module's own logic).
• crates/frontend-promql/src/promql.rs (the only front end that constructs it) and its test suites (promql_conformance.rs, observability/awesome_prometheus_alerts.rs)
• crates/frontend-sql test suites that pattern-match generically over every QueryExpr variant (netflow.rs, synthetic_packet_trace.rs)
• crates/integration-tests/tests/binary_op.rs
• crates/devtools/src/bin/variant_coverage.rs, crates/devtools/examples/canonical_examples.rs
• docs/pre-asap-ir.md

Behavior preservation

This is a pure representation refactor — no semantics change. All existing tests pass unchanged (cargo test --workspace: every crate green, including the full PromQL conformance suite, the SQL lowering suite, and the CSE/binding integration tests). Added new unit tests:

• query_expr.rs::promql_scalar_bridges_a_literal_float_at_an_operator_position — the constructor/accessor round-trip, and that a bridge and an unwrapped Literal of the same value are distinct nodes.
• query_expr.rs::row_schema_rides_on_the_bridge_wrapper_not_the_literal_variant — pins the tree-position distinction directly: the identical Literal(Float64(42.0)) value has a row schema when bridged, and still errors with ScalarHasNoRowSchema when bare.
• query_expr.rs::binary_op_schema_follows_the_vector_side_over_a_scalar_bridge_with_vector_match_intact and resolve.rs::resolve_root_threads_a_scalar_bridge_operand_and_preserves_vector_match — confirm BinaryOp's vector_match semantics (Instance 2's territory) are untouched by this change, both pre- and post-resolve_root.

What's scoped out

Instance 2 (BinaryOp vs Compare/Arithmetic) is not addressed here. BinaryOpKind already just wraps ArithmeticOpKind/CompareOpKind, so the same duplication pattern exists one level up — but BinaryOp carries vector_match: Option<VectorMatch>, which Compare/Arithmetic don't and which only makes sense at the relational-tree position, and the blast radius is roughly 60 call sites (vs. ~25 for PromqlScalar) including several PromQL-specific vector-match assertions that need care to preserve exactly. That's large enough to want its own PR. Filed as a tracking issue: #245.

Closes: part of #220 (Instance 1 only — Instance 2 tracked separately in #245)

🤖 Generated with Claude Code