Skip to content

fix(control-plane): reject cost evidence quoting another dataset - #780

Closed
zzylol wants to merge 264 commits into
split/erp-workload-costsfrom
fix/cost-evidence-dataset
Closed

zzylol wants to merge 264 commits into
split/erp-workload-costsfrom
fix/cost-evidence-dataset

Conversation

@zzylol

@zzylol zzylol commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Reject cost evidence that quotes another dataset

Stacked between #786 and #742: #728 → #786 → this PR → #742 → #775.

Before: WorkloadCostEvidence quotes are matched to candidates by full manifest equality. A quote whose dataset_identity differs from the deployment simply fails to match, so a foreign quote is silently ignored and the remaining quotes still choose the plan. Only when every quote is foreign does selection fail, and then with a generic "no feasible completely costed candidate" error.

After: WorkloadCostEvidence::validate compares every quote's manifest.dataset_identity (both namespace and dataset) with PhysicalDeploymentContext.dataset_identity. One evidence generation prices one dataset, so a single foreign quote rejects the whole generation with a typed error:

CompileError::CostEvidenceDataset { expected, found } // found = first mismatching quote

No wire-format change: evidence gains no top-level field; the identity already carried by each quote is checked.

Tests

evidence_quoting_another_dataset_is_rejected_whole covers:

  • matching evidence still selects;
  • one quote with a different namespace → CostEvidenceDataset with the expected/found identities;
  • same namespace, different dataset → same error;
  • both select_lowest_cost_candidate (PromQL) and select_lowest_cost_metricsql_candidate.

Validation

🤖 Generated with Claude Code

zzylol and others added 22 commits September 28, 2026 21:13
The split branch's tree is already contained in this branch.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@zzylol
zzylol force-pushed the test/promql-exact-function-coverage branch from 6042bb0 to 649fe08 Compare September 29, 2026 01:27
@zzylol
zzylol changed the base branch from test/promql-exact-function-coverage to split/erp-workload-costs September 29, 2026 01:27
@zzylol

zzylol commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Closing: the code this PR changes was removed by #818, which replaced the old backend with the stack that executes ASAPPlanner's selected plan.

@zzylol zzylol closed this Oct 20, 2026
zzylol added a commit that referenced this pull request Oct 21, 2026
…frontend only (#843)

fix(deps): pin ASAPPlanner 7565c47e; depend on asap-planner's PromQL frontend only

Pin both Planner crates to ASAPPlanner 7565c47e, which brings:
- #780: a gauge declared non-negative (SourceFacts::values_non_negative)
  lets a per-series quantile sketch's relative error carry through sum and
  avg, and a quantile estimate carries its samples' sign;
- #782: the SQL and MetricsQL frontends are optional features.

asap-planner is now used with default-features = false, features =
["promql"], so DataFusion and the SQL and MetricsQL frontends drop out of
the build (Cargo.lock: -1,847 lines). The sync script accepts a pin
followed by other keys, such as features.

Not yet: ASAPPlanner #787 (StoredOutput.coverage and inputs replace pane);
that adaptation comes next.
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