Conversation
zzylol
left a comment
There was a problem hiding this comment.
Plesae actually run the asapplanner and asapquery-backend to get possible physical plan output, and I will review whether they are corerct.
| readout: "sum", | ||
| root_operation: None, | ||
| }, | ||
| "spatial-topk" => ExpectedPlan { |
There was a problem hiding this comment.
This should be some CMS/CS sketches with heap?
There was a problem hiding this comment.
I ran the pinned ASAPPlanner/backend compiler and exported every compilable candidate. For topk by (label_0) (3, data), it emits an exact fallback and a backend-local CurrentSeries(data, by label_0) -> TopK(k=3) readout. The test now selects and asserts the local plan. CMS/CS heaps estimate frequency-heavy hitters, while this PromQL TopK ranks the current sample values and must retain the original series labels; using a frequency sketch directly would change the query. A certified candidate-membership sketch plus exact reranking could be a separate optimization.
There was a problem hiding this comment.
Confirmed, and the enumerator already does this — the test was the thing that was silent.
Planner exposes a CountSketchWithHeap candidate for spatial-topk over the same current-series snapshot. It is enumerated, then rejected before pricing with selected summary readout has no certified accuracy guarantee, because this fixture supplies no distinct-item bound and no score separation. The exact Sort → Limit program is what binds.
family: None used to mean the test asserted nothing here. It now declares the shape explicitly:
"spatial-topk" => &[(
"CountSketchWithHeap",
Resolution::MustReject(PolicyReason::NoCertifiedGuarantee),
)],So the heap candidate is required to be present and required to be refused for that specific reason — if Planner stops exposing it, or refuses it for a different reason, the test fails. admission/spatial-topk.admission.json records it.
A certified companion fixture is not derivable from this generator; see the reply on the quantile thread and the note in required_summary_shapes.
| root_operation: None, | ||
| }, | ||
| "temporal-rate" => ExpectedPlan { | ||
| family: Some("Increase"), |
There was a problem hiding this comment.
what's the difference between rate and increase? Should they be two nodes?
There was a problem hiding this comment.
They are separate physical roles here: the maintained Increase producer stores reset-aware counter state; the query DAG has an ExactReadout::Rate over that producer, applying the rate/window readout. The test now asserts that two-node producer/readout relationship rather than treating Increase as a Rate answer.
There was a problem hiding this comment.
ExactKind has them as distinct variants (ASAPPlanner crates/types/src/post_asap/sketch.rs):
pub enum ExactKind { Sum, Count, Min, Max, Increase, Rate, IRate }Increase is the counter-reset-aware delta; Rate is that delta divided by the window duration. So they are separate operators, not one node with a flag.
They are not two nodes in a plan, though. Every rate candidate in this fixture materializes as a single node — {"family":{"ExactAggregate":["Rate","Rate"]},"kind":"summary_agg","reduction":"PerEntity"} with a Readout{statistic:"Rate", logical_lookback_ms:"60000"} at the physical layer. The reset-aware delta is internal to the Rate accumulator rather than a separate Increase node feeding it.
Worth flagging: Increase appears in zero candidates across all ten queries, and so do Count, Min, Max and IRate. The ten-query workload only exercises Sum, Rate and quantiles. That is a real coverage gap in a branch named promql-exact-function-coverage — I think it belongs in a follow-up rather than here, but it should be a deliberate decision.
| readout: "rate", | ||
| root_operation: None, | ||
| }, | ||
| "grouped-rate" => ExpectedPlan { |
There was a problem hiding this comment.
this should be, the inner node is rate, outer node is grouped sum exact aggregation.
There was a problem hiding this comment.
Agreed. The actual selected chain is Logical Aggregate(sum by label_0) -> ExactReadout(rate) -> ReadMaterialization(Increase). The test now asserts the inner rate readout, the outer exact grouped Sum, and their edge.
There was a problem hiding this comment.
Both placements are enumerated and both bind — the inventory is not committed to one.
grouped-rate-4: query-time grouped Sum over complete per-series Rate readouts.SummaryBuild(Sum)appears only in the query DAG.grouped-rate-3: fixed-window precompute over complete per-series counter states.SummaryBuild{ExactAggregate:["Sum","Sum"]}runs at maintenance time; the query DAG is justReadout{statistic:"Sum"}.
In both, each series is finalized with PromQL Rate semantics before Sum groups the values — assert_native_grouped_rate checks that the Sum consumes a Readout{statistic:"Rate", logical_lookback_ms:"60000"}, so summing raw counters before per-series rates would fail.
The test now requires both placements to appear (grouped_rate_placements must contain {false, true}), rather than asserting one expected shape. Which of the two wins is #742;'s call, not this layer's.
One defect is visible here: a third precompute-Sum variant fails with Planner logical fragment does not match any original query subtree, and a fourth with a schema incompatibility. main is green on that path, so both are regressions from inside this stack. They are pinned in KNOWN_BINDING_DEFECTS with exact counts so they cannot be fixed or worsened silently.
There was a problem hiding this comment.
Correction to my earlier reply on this thread.
I said the two bind_failed candidates here were regressions introduced inside the #737 → #728 stack, because main is green on these paths. That was wrong, and I should have diffed before asserting it.
residual_nodes is substantively identical on main. The only things this stack changed in that function are how the accuracy target is derived (previously hardcoded Exact) and the wording of the error message. main is green because its tests never plan these queries, not because the code is correct. The defect is older and wider than I claimed.
Root cause, now diagnosed rather than guessed. residual_nodes locates the query-time fragment by re-parsing every subtree of the original and comparing typed IR with ==. For this query the two sides differ in exactly one field:
residual leaf scan columns: [ts, value, label_0]
candidate leaf scan columns: [ts, value]
Everything else is identical: reduction, measures, output_names, range, source, predicates, time_index. Planner documents a PromQL leaf schema as usage-derived, "the (ts, value) floor + the labels the query references", and marks it closed: false because it deliberately does not enumerate the row. rate(data[1m]) carries label_0 only because the enclosing sum by (label_0) references it; re-parsing that subtree on its own cannot know that.
Fixed on main in #781 by comparing open leaf schemas by containment (a prefix, so positional ColumnIds keep their meaning) while everything else stays exact. The pre-existing false-match guards, which discriminate on matchers, range and metric name, all pass unchanged.
KNOWN_BINDING_DEFECTS and the review README have been corrected to say this.
| readout: "rate", | ||
| root_operation: Some("aggregate"), | ||
| }, | ||
| "grouped-temporal-sum" => ExpectedPlan { |
There was a problem hiding this comment.
this should be fused by ASAPPlanner into one sum, over time and spatial dimensions.
There was a problem hiding this comment.
The actual Planner candidate is currently Logical Aggregate(sum by label_0) -> ExactReadout(sum) -> per-entity Sum producer; it has not fused time and spatial aggregation into one grouped temporal Sum producer. I changed the expected contract to the fused producer, so the level-1 test now fails and names this gap. This comment remains an implementation blocker rather than being marked resolved.
There was a problem hiding this comment.
The fused candidate exists and is the one that binds cheapest — grouped-temporal-sum-0 is a single operator over both dimensions:
{"kind":"summary_agg","family":{"ExactAggregate":["Sum","Sum"]},"reduction":{"Reduce":[2]}}with "window_size":60 (the 1m temporal range) and "grouping_labels":{"labels":["label_0"]} (the spatial grouping) on that same node, "partitioning":"grouped". No separate per-entity stage.
The unfused shape is also enumerated (grouped-temporal-sum-3): summary_agg{Sum, reduction:"PerEntity"} → FinalizeExactAccumulator → a distinct Aggregate{sum, Reduce} node. Both are legal frontiers, so Level 1 keeps both and asserts neither as the winner.
On your earlier point about the 1-minute read: the grouped branch now checks readout_lookback_ms = 60_000 and that window_ms matches the materialization's stored_window_ms(), so a 5-second pane cannot pass as the 1-minute answer.
Also here: one candidate fails with Planner logical fragment does not match any original query subtree, recorded in KNOWN_BINDING_DEFECTS.
| readout: "sum", | ||
| root_operation: Some("aggregate"), | ||
| }, | ||
| "topk-rate" => ExpectedPlan { |
There was a problem hiding this comment.
There should be inner node of rate, outter node for some sketches for topk
There was a problem hiding this comment.
The actual local chain is TopKSelection(k=3, by label_0) -> ExactReadout(rate) -> ReadMaterialization(Increase). It computes rate first and then selects TopK locally. A CMS/CS frequency heap is not itself an exact TopK of rate values; a sketch candidate stage would require a completeness certificate and exact rerank. The test asserts the current semantically correct local chain, and all candidate plans are exported for review.
There was a problem hiding this comment.
That shape is enumerated — four candidates of it, in fact:
inner Readout{statistic:"Rate"} → outer KeyedSummaryBuild{Sketch:[CmsWithHeap | CountSketchWithHeap]} → KeyedReadout → Sort → Limit.
Two are query-time, two are fixed-window candidates that finalize Rate and build the heap during precompute. All four are rejected before pricing with no certified accuracy guarantee, so exact Sort → Limit binds.
The test now requires both families and that specific resolution:
"topk-rate" => &[
("CmsWithHeap", Resolution::MustReject(PolicyReason::NoCertifiedGuarantee)),
("CountSketchWithHeap", Resolution::MustReject(PolicyReason::NoCertifiedGuarantee)),
],plus the pre-existing structural checks that the maintenance DAG contains Rate + KeyedSummaryBuild while the query DAG contains KeyedReadout and not KeyedSummaryBuild, and that ranking sorts on the finalized rate value rather than a timestamp or label.
| readout: "rate", | ||
| root_operation: Some("top_k_selection"), | ||
| }, | ||
| "quantile-ratio" => ExpectedPlan { |
There was a problem hiding this comment.
this should be DDSketch / DDSketch
There was a problem hiding this comment.
Agreed on the intended composition: q=0.9 and q=0.5 quantile-sketch readouts over the same per-series window, followed by local division. I added that target DAG contract. The current Planner emits only ExactFallback candidates, so the level-1 test remains red for this query; I am not treating fallback as a correct answer.
There was a problem hiding this comment.
That is what happens, and the test now says so instead of asserting nothing.
quantile-ratio-0 is the bound candidate: one summary_agg{Sketch:[DDSketch,...]} feeding two summary_estimate{Quantile} nodes (q=0.9 and q=0.5) into a binary/Div. So DDSketch / DDSketch, sharing a single producer.
KLL is attempted and discarded before admission — its composed guarantee does not satisfy EpsilonDelta{epsilon:0.01, delta:0.01} for either quantile. The other two candidates degrade to exact_fallback as a result. Exact (non-fallback) never appears.
The existing assertions already check the operand structure, the shared per-entity materialization at readout_lookback_ms = 60_000, and that both readouts come from a quantile-sketch family. What changed is that a KLL rejection for any reason other than an accuracy-target miss now fails the test rather than passing quietly.
|
I ran the pinned ASAPPlanner through the backend's real candidate enumeration and physical compiler for all ten issue #754 queries. The test exports both selected and every compilable candidate as JSON/DOT through the PR #736 renderer; the current CI run is https://github.com/ProjectASAP/ASAPQuery-backend/actions/runs/35736247732 (artifact
The test now checks the specific DAG contracts and stays red for the two highlighted gaps. These are actual compiler outputs for the single-query level-1 snapshots with deterministic test costs, not measurements of production cost selection or data-plane execution. #742 is the separate runtime/semantic gate. |
|
For The planning objective is minimum execution cost for the workload, accounting for maintenance CPU, retained state, query CPU/read volume, update cadence, and sharing across queries. Hard-coding this split as the definition of plan correctness could reject a cheaper, semantically valid candidate. The current deterministic test costs also do not establish a production cost optimum. Could we make the correctness contract assert rate-before-grouped-sum semantics, the window/evaluation-time and population contracts, and writer/read bindings independent of where the Sum executes? Then separately test that explicit workload/cost evidence selects the cheapest admitted executable candidate, and use Level 2 to compare the selected plan's results. If this shape is intentionally pinned only for this fixture, please label it as a fixture-specific selection expectation rather than a general correctness requirement. |
|
Next query: Separately, this test deliberately fails the current per-series Sum → readout → query-time grouped Sum plan and requires a fused grouped temporal Sum producer. Fusion is a valid candidate, but the per-series plan can also be semantically correct. Given the goal of minimum workload execution cost, I would test both as admissible when their coverage and bindings are valid, then use explicit workload/cost evidence to select the cheaper one. Please keep a fixed fused shape only if this is a fixture-specific cost-selection expectation, not the general correctness definition. |
|
Concrete change proposed for this Level-1 acceptance test and the planning path (following the grouped-rate/grouped-temporal-sum comments):
This seems consistent with Planner #462 owning deployment-independent physical lowering, #761 owning backend workload costing/evidence, and #737/#749 binding selected producer/read plans through stored output identity. Does this match the intended ownership? |
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> (cherry picked from commit cebfb60)
…needs issue754_workload is `#[path]`-included by every level's test binary, so each binary compiles the whole module while calling only the helpers that level needs. Under `-D warnings` that turns an unused helper into a hard error in whichever level does not call it: `certified_topk_input`, added for level 1's certified heap fixture, broke the level 2 binary in #742. The lint is about the including binary, not about the fixture, so allow it at the module level and say why. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The workload-snapshot version was restated at every site that produced or consumed a snapshot: the backend's check, two o11y tools, a shared-workload tool, the shipped example snapshots and their tests. Nothing tied them together, so they drifted. On the #728 stack two tools still overwrote the version with a stale literal of 2 while the backend had moved to 3, which meant a snapshot built from a current template was relabelled to an old version and then rejected by the very backend that had produced the template. Name it once, as WORKLOAD_SNAPSHOT_VERSION beside the field it governs, and use it for both the check and its message. Producers are written in Rust, Python and JSON and cannot share a constant, so tie them together with a test instead: the shipped snapshots must declare that version, and each tool must reject anything else and must not relabel what it is handed. A tool changes a snapshot's content, not its schema, so it has no business restating the version; that is precisely how the literal went stale. Bumping the schema is now one edit plus whatever the test reports, rather than a literal that some producers follow and others quietly do not. Verified by mutation: raising the constant to 4 fails both tests, and reintroducing a `snapshot["snapshot_version"] =` write into a tool fails with the message naming that tool. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review candidate correctness independently of cost ranking.
Before this PR: workload tests selected a synthetic-cost winner and mixed structural checks with ranking assertions. Valid Planner computation could also be rejected when a Backend binding lost its semantics.
After this PR: Level 1 enumerates the supported candidate inventory before pricing for ten individual queries and three ensembles: shared-rate, shared-quantiles, and the full workload. It checks typed DAG dependencies, windows, grouping, Rate sort expressions, materialization boundaries, dataset-bound SDS definitions, and shared producer identity. A complete Rate frontier may itself be a physical output; its identity program is checked explicitly.
There is no binding-defect allowlist. Unexpected compilation failures fail the suite. The strict fixture explicitly rejects heap candidates without certified accuracy evidence; the companion fixture proves those families bind when the required evidence is supplied. Every admission record remains unpriced.
Validation: all four Level 1 tests pass, including the wrong-sort-key mutation, certified heap admission, and ensemble checks. Admission reports and review instructions are updated. Complete candidate JSON/DOT and ensemble exports are generated as CI artifacts, not committed bundles. These exports support human review and do not constitute human approval.
Review sequence: #728 checks structure; #742 checks synthetic-cost ranking; #775 installs selected candidates and executes the data plane. Candidate search has an explicit bounded scope. Real workload/resource measurements, online ERP feedback, and runtime replanning remain deferred.