Skip to content

test: assert issue 754 level-1 physical DAGs for every query - #728

Open
zzylol wants to merge 27 commits into
refactor/typed-summary-plan-supportfrom
test/promql-exact-function-coverage
Open

zzylol wants to merge 27 commits into
refactor/typed-summary-plan-supportfrom
test/promql-exact-function-coverage

Conversation

@zzylol

@zzylol zzylol commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

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.

@zzylol zzylol changed the title feat(promql): add executable exact bindings and full function smoke coverage test: verify issue 754 level-1 physical plans Sep 22, 2026

@zzylol zzylol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be some CMS/CS sketches with heap?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread control_plane/tests/issue754_level1.rs
Comment thread control_plane/tests/issue754_level1.rs
Comment thread control_plane/tests/issue754_level1.rs Outdated
root_operation: None,
},
"temporal-rate" => ExpectedPlan {
family: Some("Increase"),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference between rate and increase? Should they be two nodes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be, the inner node is rate, outer node is grouped sum exact aggregation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be fused by ASAPPlanner into one sum, over time and spatial dimensions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be inner node of rate, outter node for some sketches for topk

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be DDSketch / DDSketch

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@zzylol

zzylol commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

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 issue754-level1-plans is uploaded even when the strict test fails).

Query Current cost-selected query DAG
spatial sum grouped Sum producer → Sum readout
spatial TopK backend-local CurrentSeries(data, by label_0) → TopK(k=3) readout
spatial quantile grouped DDSketch → q=0.9 readout
temporal sum per-series Sum → Sum readout
temporal quantile per-series DDSketch → q=0.9 readout
temporal rate per-series Increase → Rate readout
grouped rate per-series Increase → Rate readout → exact Sum by label_0
grouped temporal sum per-series Sum → Sum readout → exact Sum by label_0 (not the requested fused producer)
TopK rate per-series Increase → Rate readout → local TopK(k=3, by label_0)
quantile ratio ExactFallback (no local candidate)

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.

@zzylol zzylol changed the title test: verify issue 754 level-1 physical plans fix: preserve Planner families in issue 754 level-1 plans Sep 22, 2026
@zzylol zzylol changed the title fix: preserve Planner families in issue 754 level-1 plans refactor: align physical DAG, SDS, catalog, and runtime with Planner families Sep 22, 2026
@zzylol zzylol changed the title refactor: align physical DAG, SDS, catalog, and runtime with Planner families test: assert issue 754 level-1 physical DAGs for every query Sep 22, 2026
@zzylol
zzylol changed the base branch from main to fix/precompute-post-asap-dag September 22, 2026 16:16
@zzylol
zzylol changed the base branch from refactor/backend-plan-split to refactor/typed-summary-plan-support September 24, 2026 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant