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.
| 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.
| 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.
| 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.
| 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.
| 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.
|
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. |
Dependency stack: main → #768 → #737 → #749 → #771 → #728 → #770 → #763 → #765 → #761 → #742 → #759
Independent follow-ups to #765: #756 (diagnostics), #766 (runtime controls and overhead inspection).
Before this PR
The issue #754 acceptance work mixed production implementation with physical-plan assertions. Production changes and their coupled regression tests are now owned by #771.
After this PR
This PR changes exactly four files:
control_plane/tests/issue754_level1.rs, the issue-754 query suite, its dataset and the CI configuration that runs the gate. It validates explicit producer/readout/reduction dependencies, catalog/SDS agreement and local execution. It does not change production Rust code.Stacked directly on #771. #770 follows with the Planner-owned library integration.
Validation and remaining gates
Combining #771 and this PR reproduces the original #728 Git tree exactly; #770/#763/#765 retain their prior trees after ancestry propagation. The mandatory grouped-temporal-Sum and quantile-ratio expectations remain unchanged and still fail at the end of the stack. No assertion is weakened by the split.