Repository navigation
fix(control-plane): match a Planner fragment against its own subtree - #781
Merged
Merged
Conversation
residual_nodes locates the part of a query that still runs at query time by re-parsing every subtree of the original and comparing the typed IR with `==`. That comparison includes the leaf scan's schema, and the schema is the one thing in the tree that cannot survive the round trip. Planner documents a PromQL leaf schema as usage-derived: "the (ts, value) floor + the labels the query references", marked `closed: false` precisely because it does not enumerate the row. A fragment resolved inside the whole query therefore carries every label the query mentions, while the same fragment re-parsed alone carries only the labels it mentions. So `sum by (label_0) (rate(data[1m]))` produces a residual whose leaf scan has columns [ts, value, label_0], while re-parsing the subtree `rate(data[1m])` produces [ts, value]. Same source, predicates, range, measures, reduction; one context-derived column apart. The match failed and the query was rejected with "Planner residual does not match any original query subtree", even though the fragment is that subtree. Compare open leaf schemas by containment instead. Containment is a prefix, not an arbitrary subset, because ColumnIds are positional: the floor comes first and context only appends, so a prefix keeps every column id in predicates and grouping keys meaning the same column on both sides. Closed catalog-backed SQL schemas do enumerate the row, so they keep exact equality, and every other part of the tree is still compared exactly. Nothing that identifies the computation is relaxed. The existing false-match guards discriminate on matchers, range and metric name, and all still pass; the new test checks a different metric, range, function and matcher are each still rejected under a widened leaf schema. Found while auditing bind failures in the issue-754 workload, where this rejected three single-query candidates and, in batch planning, fifteen across a ten-query workload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Sep 28, 2026
An earlier revision claimed every recorded defect was a regression introduced inside the #737 -> #728 stack, on the grounds that main is green on these paths. Diffing residual_nodes against main does not support that: the function is substantively identical there, and the stack changed only how the accuracy target is derived and the wording of the error message. main is green because its tests never plan these queries, not because the code is correct. That is the more worrying reading, so it should be the recorded one. The fragment mismatch is fixed on main by #781: a context-derived leaf schema was compared for equality against an isolated re-parse that cannot reproduce it, and open schemas are now compared by containment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The new tests were inserted between an existing `#[test]` and the function it applied to, so `grouped_query_residual_matches_its_own_subtree` carried two attributes and `workload_horizon_residual_keeps_semantic_equality` silently stopped being a test. A local `cargo test --lib` run hid both, since neither lint is an error without `-D warnings`. Move the new tests above the attribute and give the original its own back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Sep 28, 2026
#781 is on main: an open PromQL leaf schema is context-derived, so comparing it for equality against an isolated re-parse rejected fragments that were the subtree. With that fixed, every recorded fragment mismatch is gone: three single-query candidates, one in shared-rate and fifteen in all-ten. Removing them exposed a second schema incompatibility in all-ten that had been hidden behind the earlier rejection, so that count goes from 1 to 2. That is what pinning exact counts is for; a test that only checked "some non-empty reason" would have absorbed both the fix and the newly reachable defect without saying anything. Verified on this branch with the fix applied: level 1 is green on all four tests, and the two data_plane dashboard tests that failed with the same fragment-mismatch error, repeated_dashboard_executes_multiple_selected_panes and shared_exact_dashboard_executes_selected_workload, both pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Oct 20, 2026
zzylol
added a commit
that referenced
this pull request
Oct 20, 2026
SummaryStore keys an instance by SummaryInstanceKey { definition,
selection }, with a time-only Selection ((end - width, end]) for now. This
is ASAPPlanner's summary coverage (definition plus selection) restricted
to time selections; when StoredOutput carries the coverage (ASAPPlanner
#781), the SDS takes the plan's definition id and selection instead of its
own hash and the coverage end.
zzylol
added a commit
that referenced
this pull request
Oct 20, 2026
…s as summaries (#838) feat!: install the Planner's InstallablePlan; store its stored outputs as summaries The control plane calls asap_planner::plan_installable once and writes the returned InstallablePlan as plan.json; the backend's own Stage 0-3 plumbing, pane-chain expansion and series-identity rewrite are gone. The SDS stores immutable summaries for every stored output the plan lists, built in an ingestion run or in a query run and kept: Sds::from_plan groups outputs by the SDS's own identity (sub-DAG hash, offset-only TimeShifts transparent), records which run builds each definition, keys summaries by (definition, coverage end), and keeps each definition for the largest retention_ms among its outputs (the Planner's retention, which includes range-evaluation spans) instead of "largest offset plus two panes". Each definition lists the stored outputs its builder reads, and Sds::ingestion_build_order orders ingestion-run definitions after their stored inputs. The precompute engine builds ingestion-run definitions in that order, from a RawSampleSelector over the plan's coverage window, and refuses one built from other stored outputs until it can build it once its inputs are sealed. Query-run definitions are left to the query engine; the backend still declares query_time_results_retained: false until that lands. Among Planner crates the workspace depends on asap-planner and asap-executor only, at ASAPPlanner main ecea897f; IR types come through asap_executor::planner. The sync script, its test and the CI README update both. asap-planner pulls in the SQL and MetricsQL frontends until they become optional features upstream. fix(precompute): measure retention from the newest closed pane An evaluation stored panes can serve is at most at the end of the newest closed pane, not at the watermark: with the watermark at 74:45 and Example 4b's 60-minute retention, the pane ending at 20 min is still read by the evaluation at 70 min, but was skipped as expired. refactor(sds): key summary instances by definition and selection SummaryStore keys an instance by SummaryInstanceKey { definition, selection }, with a time-only Selection ((end - width, end]) for now. This is ASAPPlanner's summary coverage (definition plus selection) restricted to time selections; when StoredOutput carries the coverage (ASAPPlanner #781), the SDS takes the plan's definition id and selection instead of its own hash and the coverage end. fix(sds): keep one pane beyond the retention; order and check stored inputs - Panes are kept for one extra pane width beyond the plan's retention, as the design doc describes: with the watermark at 84:45 (newest closed pane 80 min), the evaluation at 70 min lost its pane ending at 20 min. The expired-pane skip uses the same bound. - Sds::ingestion_build_order refuses an ingestion-run definition that reads a query-run one, which is built only when an evaluation needs it. - PaneBinding.raw_input is removed: nothing here reads it.
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.
residual_nodesmaps the query-time part of a plan back onto a subtree of the original query by re-parsing each subtree and comparing typed IR with==. The comparison includes the leaf scan schema, which is the one part of the tree that cannot survive that round trip.Planner documents a PromQL leaf schema as usage-derived, "the
(ts, value)floor + the labels the query references", and marks itclosed: falsebecause it deliberately does not enumerate the row. A fragment resolved inside the whole query carries every label the query mentions; the same fragment re-parsed alone carries only the labels it mentions.Concretely, for
sum by (label_0) (rate(data[1m])):Everything else is identical: reduction, measures, output_names, range, source, predicates, time_index. The subtree
rate(data[1m])was rejected as "not any original query subtree" although it is exactly that subtree.Change
Open leaf schemas are compared by containment. Containment is a prefix, not an arbitrary subset, because
ColumnIds are positional: the floor comes first and context only appends, so a prefix keeps every column id inpredicatesand grouping keys meaning the same column on both sides. Closed (catalog-backed SQL) schemas still require exact equality, and every other node is compared exactly.Why this is not a loosening
Nothing that identifies the computation is relaxed. The pre-existing false-match guards discriminate on matchers (
job="api"vs"worker"), range ([1m]vs[2m]) and metric name, and all pass unchanged. The new test asserts that under a widened leaf schema a different metric, a different range, a different function and a different matcher are each still rejected.Validation
cargo test -p control_plane --lib: 433 passed, 0 failed.cargo fmt --checkandcargo clippy -p control_plane --libclean.Provenance
Found while auditing bind failures in the issue-754 workload. It rejected three single-query candidates there, and fifteen in batch planning across a ten-query workload. The defect is on
main;mainis green only because its tests do not plan these queries.🤖 Generated with Claude Code