fix(l2): resolve outer group keys (incl. __name__) over binary-op sides (#52) - #104
Merged
Merged
Conversation
… sides (#52) `sum by (__name__)(a or b)` — and the general `sum by (job)(a or b)` — failed column resolution: `column __name__ not found in schema`. A binary op's two sides each re-run the Binder against their own sub-tree (so different metrics bind their labels to the right positions), which means a group key referenced only by an *enclosing* aggregate is invisible when binding either side. `__name__` (the metric-name label, present on every series) was the reported instance, but any outer key hit it. Fix: when converting a `BinaryOp`, seed the ancestor-referenced names into each side's binding via the new `Binder::bind_with_inherited`. The inherited set is the enclosing scope's referenced columns (already on `fallback`) minus the names referenced *within* the binary op itself — subtracting the op's own refs is what stops one side's labels leaking into the other (which would, and initially did, change unrelated exact-tree lowerings). SQL is unaffected: table scans carry their own catalog schema and ignore the usage-derived fallback. Tests: a Binder unit test for `bind_with_inherited`; a conformance test (both `or` sides carry `__name__` at a consistent position that the outer key resolves to, plus the general `job` case); an exact-tree e2e pin of the issue's repro; and a note in docs/promql-lowering.md. Existing binary-op exact-tree tests are unchanged (no sibling-label leak). Closes #52 Co-Authored-By: Claude Fable 5 <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
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 #52.
The bug is more general than the title
sum by (__name__)(metric_total{env="1"} or rate(metric_total{env="2"}[5m]))failed withcolumn __name__ not found in schema (have: ["ts", "value", "env"]). But it isn't__name__-specific —sum by (job)(metric_a or metric_b)fails identically. Any outer aggregate group key that a binary op's sides don't reference in their own matchers hits it.Root cause. A
BinaryOp's two sides re-run the Binder against their own sub-tree (convert_rootper side), because the branches may scan different metrics with different label sets and each side's columns must bind to positions in its own leaf — threading the parent schema would bind the right side's columns to the wrong positions. The cost: a key referenced only by an enclosing node (the outersum by (…)) is invisible when binding either side, so it fails to resolve against the binary op's output.__name__was the reported instance because it's the metric-name label present on every series, but the mechanism is general.Fix
When converting a
BinaryOp, seed the inherited (ancestor-referenced) names into each side's binding, via the newBinder::bind_with_inherited. The inherited set is computed precisely:Subtracting the binary op's own references is the load-bearing part: it yields exactly the ancestor keys and stops one
orside's labels from leaking into the other. My first cut seeded all offallback's labels and regressed an existing exact-tree test (q25_div_over_complex_subtrees) by leaking a siblingstatuslabel across branches — the subtraction fixes that, and that test now passes unchanged.Both independently-bound sides end up carrying the inherited key at the same position, so the outer group key resolves consistently across the union.
SQL is unaffected: table scans carry their own catalog schema (
Source::Table) and ignore the usage-derived fallback, so the seeding is a no-op for them — confirmed by the unchanged SQL suite.Tests
bind_with_inherited(seeds the inherited name; plainbinddoesn't conjure it).outer_group_key_over_binary_op_resolves_on_both_sides: the issue's__name__-over-orcase resolves to a single positional id, both sides carry__name__at the same position matching the outer key, and the generalsum by (job)(a or b)case lowers.q52_outer_name_label_over_binary_opof the repro.docs/promql-lowering.md.q25, thebinary_op.rssuite) are unchanged — no sibling-label leak.Notes
asap-l2only; independent of other open work.cargo test --workspace— 0 failures;cargo clippy --all-targetsclean; source files carry zero net-newcargo fmtdrift vs main.🤖 Generated with Claude Code