From c4dab0220a94ac722063d3db2ac420464bb56973 Mon Sep 17 00:00:00 2001 From: Milind Srivastava Date: Mon, 2 Nov 2026 13:47:53 -0500 Subject: [PATCH] fix(plan-selection): reject an unpriceable candidate in the reference pricer The search rejects a candidate whose cost overflows (cost_not_representable), but the test-only reference pricer failed the whole selection on it. Which round the search selects depends on timing, so planner_layering_example2's production_memo_selects_a_validated_example2_plan failed whenever the selected round held such a candidate. The reference pricer now rejects it with the new RejectionCause::CostNotRepresentable. Co-Authored-By: Claude Opus 5.5 --- crates/devtools/src/bin/stage_pipeline.rs | 3 +- crates/devtools/tests/viewer_contract.rs | 2 + crates/plan-selection/src/lib.rs | 37 +++++++++++++++++++ crates/plan-selection/src/report.rs | 8 +++- crates/plan-selection/src/selector.rs | 27 ++++++++++++-- .../asap-aware-mapping-contracts.md | 4 +- tools/dag-viewer/README.md | 3 +- tools/dag-viewer/stages.js | 2 +- 8 files changed, 76 insertions(+), 10 deletions(-) diff --git a/crates/devtools/src/bin/stage_pipeline.rs b/crates/devtools/src/bin/stage_pipeline.rs index 5d2f44bfb..424afd90b 100644 --- a/crates/devtools/src/bin/stage_pipeline.rs +++ b/crates/devtools/src/bin/stage_pipeline.rs @@ -109,7 +109,8 @@ // `cause` (a failed check: `accuracy_target_missed`, // `count_min_weights_not_proven_non_negative`, `no_accuracy_guarantee`, // `latency_bound_exceeded`, `latency_over_repeat_interval`, -// `over_cluster_capacity`; or a loss under the cost model: `costlier`, +// `over_cluster_capacity`, `cost_not_representable`; or a loss under +// the cost model: `costlier`, // `slower`, `more_memory`, `tied_lost_on_enumeration_order`), // `pruned_candidates_not_enumerated` (rejections naming no enumerated // candidate) and `pruned_candidate_count_by_cause`. diff --git a/crates/devtools/tests/viewer_contract.rs b/crates/devtools/tests/viewer_contract.rs index 23680f0f4..6bf8d3ccb 100644 --- a/crates/devtools/tests/viewer_contract.rs +++ b/crates/devtools/tests/viewer_contract.rs @@ -354,6 +354,7 @@ fn viewer_knows_every_view_refresh_and_rejection_cause() { C::LatencyOverRepeatInterval, C::OverClusterCapacity, C::Infeasible, + C::CostNotRepresentable, C::Costlier, C::Slower, C::MoreMemory, @@ -368,6 +369,7 @@ fn viewer_knows_every_view_refresh_and_rejection_cause() { | C::LatencyOverRepeatInterval | C::OverClusterCapacity | C::Infeasible + | C::CostNotRepresentable | C::Costlier | C::Slower | C::MoreMemory diff --git a/crates/plan-selection/src/lib.rs b/crates/plan-selection/src/lib.rs index 04c7f6c05..8e4e708c1 100644 --- a/crates/plan-selection/src/lib.rs +++ b/crates/plan-selection/src/lib.rs @@ -5691,6 +5691,43 @@ mod tests { } } + /// The reference pricer rejects a candidate it cannot price, as the + /// search does, rather than failing the whole selection. + #[test] + fn the_reference_pricer_rejects_an_unpriceable_candidate() { + let candidates = tumbling_window_candidates(10_000.0); + let kll = candidates[0] + .dag + .nodes + .iter() + .find_map(|node| match &node.payload { + Payload::ASAP(ASAPOp::SummaryTumblingWindowChain { + family: FieldDataType::Sketch(kind, _), + .. + }) => Some(kind.params().clone()), + _ => None, + }) + .unwrap(); + let settings = measured(vec![OperatorCost { + memory_bytes_per_state: Some(u64::MAX as f64), + ..sketch_entry(OperatorKind::SummaryTumblingWindowChain, kll, 1.0) + }]); + let entries = [entry_with(p99_target(), repeating(MINUTE))]; + let models = SelectionModels { + cost_inputs: &settings, + ..SelectionModels::builtin() + }; + let Err(ErrorSelectingPlan::NoValidCandidate(rejected)) = + select_among_plans(&candidates, &entries, &arriving(10_000.0), models) + else { + panic!("every candidate overflows, so none is valid"); + }; + assert_eq!(rejected.len(), candidates.len()); + assert!(rejected + .iter() + .all(|r| r.cause == RejectionCause::CostNotRepresentable)); + } + /// The table of earlier versions, a JSON array of sketch operations, /// still loads: each entry becomes its operator's entry at both phases, /// an update both a `SummaryAgg` and a `SummaryTumblingWindowChain` entry. diff --git a/crates/plan-selection/src/report.rs b/crates/plan-selection/src/report.rs index b06ec740a..78a5db077 100644 --- a/crates/plan-selection/src/report.rs +++ b/crates/plan-selection/src/report.rs @@ -74,6 +74,9 @@ pub enum RejectionCause { LatencyOverRepeatInterval, /// Under cost model 3, its usage is over one of the cluster's limits. OverClusterCapacity, + /// Its cost overflows the cost model's arithmetic, so it cannot be + /// priced. + CostNotRepresentable, /// Under cost models 1 and 2, its price is higher than the selected /// candidate's. Costlier, @@ -94,8 +97,8 @@ pub enum RejectionCause { impl RejectionCause { /// Whether the candidate failed a check (accuracy, latency bound, - /// repeat interval or cluster capacity) rather than losing under the - /// cost model. + /// repeat interval, cluster capacity or a representable cost) rather + /// than losing under the cost model. pub fn failed_a_check(self) -> bool { matches!( self, @@ -106,6 +109,7 @@ impl RejectionCause { | Self::LatencyOverRepeatInterval | Self::OverClusterCapacity | Self::Infeasible + | Self::CostNotRepresentable ) } } diff --git a/crates/plan-selection/src/selector.rs b/crates/plan-selection/src/selector.rs index d1a289bc7..d30aa65ac 100644 --- a/crates/plan-selection/src/selector.rs +++ b/crates/plan-selection/src/selector.rs @@ -17,7 +17,7 @@ use crate::checks::{accuracy_check, accuracy_targets, latency_violation}; use crate::report::{ AcceptedAccuracy, ErrorSelectingPlan, Rejection, RejectionCause, WholePlanSelection, }; -use crate::SelectionModels; +use crate::{AnalyticalCostError, SelectionModels}; use asap_cost::{ kept_tumbling_window_chains, node_map, plan_cost, query_reach, tumbling_window_retention_ms, }; @@ -57,12 +57,33 @@ pub(crate) fn select_among_plans( if selector.needs_calibration() { let mut fit = CalibrationFit::default(); for candidate in cands { - fit.add(selector.calibration_fit(candidate)?); + match selector.calibration_fit(candidate) { + // Rejected below when offered. + Err(ErrorSelectingPlan::Cost { + error: AnalyticalCostError::Overflow, + .. + }) => {} + fit_or_error => fit.add(fit_or_error?), + } } selector.set_calibration(fit); } + // An unpriceable candidate is rejected, as the search rejects it + // (`cost_not_representable`), rather than failing the selection. for candidate in cands { - selector.offer(candidate)?; + match selector.offer(candidate) { + Err( + error @ ErrorSelectingPlan::Cost { + error: AnalyticalCostError::Overflow, + .. + }, + ) => selector.reject( + &candidate.id, + RejectionCause::CostNotRepresentable, + error.to_string(), + ), + result => result?, + } } selector.finish().map(|(selection, _)| selection) } diff --git a/docs/develop_docs/asap-aware-mapping-contracts.md b/docs/develop_docs/asap-aware-mapping-contracts.md index bcefc170c..6086507c2 100644 --- a/docs/develop_docs/asap-aware-mapping-contracts.md +++ b/docs/develop_docs/asap-aware-mapping-contracts.md @@ -112,8 +112,8 @@ asap-physical-asap-optimization -p asap-plan-selection -p asap-types `CountMinWeightsNotProvenNonNegative`, `NoAccuracyGuarantee`, `LatencyBoundExceeded`, `LatencyOverRepeatInterval` (a repeating query's latency per evaluation is over its repeat interval, - checked with or without a latency bound), or `OverClusterCapacity` under - cost model 3) or a loss under the cost model (`Costlier`, `Slower`, + checked with or without a latency bound), `OverClusterCapacity` under + cost model 3, or `CostNotRepresentable` (its cost overflows)) or a loss under the cost model (`Costlier`, `Slower`, `MoreMemory`, `TiedLostOnEnumerationOrder`). Stage 3 never resizes a summary: a candidate whose Stage 1 sizes miss its composed accuracy target is rejected alone. - Usage (`ResourceUsage`) is measured without prices; the `CostModel` in diff --git a/tools/dag-viewer/README.md b/tools/dag-viewer/README.md index 1b5445b68..15d0a2c8a 100644 --- a/tools/dag-viewer/README.md +++ b/tools/dag-viewer/README.md @@ -337,7 +337,8 @@ are ignored, `stage_pipeline`'s default. (`accuracy_target_missed`, `count_min_weights_not_proven_non_negative`, `no_accuracy_guarantee`, `latency_bound_exceeded`, - `latency_over_repeat_interval`, `over_cluster_capacity`) or a loss under + `latency_over_repeat_interval`, `over_cluster_capacity`, + `cost_not_representable`) or a loss under the cost model (`costlier`, `slower`, `more_memory`, `tied_lost_on_enumeration_order`). - `accuracy_guarantees` is `ignored` (`stage_pipeline`'s default: Stage 3 diff --git a/tools/dag-viewer/stages.js b/tools/dag-viewer/stages.js index ac7dd25a0..1470eba23 100644 --- a/tools/dag-viewer/stages.js +++ b/tools/dag-viewer/stages.js @@ -313,7 +313,7 @@ const FAILED_CHECK_CAUSES = [ 'accuracy_target_missed', 'count_min_weights_not_proven_non_negative', 'no_accuracy_guarantee', 'latency_bound_exceeded', 'latency_over_repeat_interval', - 'over_cluster_capacity', 'infeasible', + 'over_cluster_capacity', 'infeasible', 'cost_not_representable', ]; // Every `reference_pricing.pruned_candidates[].cause` the reference pricer // writes: the failed checks, then the losses under the cost model (priced,