test(plan-selection): pin that the hook never overrides a guarantee that misses

Add a Stage 3 test with evidence that misses the target and an
always-accepting hook (still AccuracyTargetMissed), and an end-to-end
test that candidates whose guarantee misses under the built-in models stay
rejected under an accepting model. Drop the unused crate-root re-export of
PropagationStats and re-indent the accuracy rejection reasons.

Author: zzylol

crates/logical-asap-optimization/src/lib.rs

@@ -54,7 +54,6 @@ pub mod realization;
 #[cfg(any(test, feature = "test-support"))]
 pub mod test_support;
 
-pub use accuracy::PropagationStats;
 pub use deployment_capabilities::{
     DeploymentCapabilities, DeploymentCapabilitiesError, SupportedSummary,
 };

crates/plan-selection/src/lib.rs

@@ -1534,24 +1534,23 @@ fn accuracy_check(
                 {
                     format!(
                         "q{query}: the output's guarantee holds for each value, and the number \
-                     of values one evaluation returns is not bound (no declared series count \
-                     for a source a PromQL vector reads; no bound on a SQL result's groups \
-                     yet), \
-                     so it does not meet whole-result target {target:?}, and the deployment \
-                     has no evidence that it meets it"
+                         of values one evaluation returns is not bound (no declared series \
+                         count for a source a PromQL vector reads; no bound on a SQL result's \
+                         groups yet), so it does not meet whole-result target {target:?}, and \
+                         the deployment has no evidence that it meets it"
                     )
                 }
                 Some(g) => format!(
-                    "q{query}: the output guarantees a {:?} bound {:?}, failure probability {:?}, \
-                 which misses target {target:?}, and the deployment has no evidence that it \
-                 meets it",
+                    "q{query}: the output guarantees a {:?} bound {:?}, failure probability \
+                     {:?}, which misses target {target:?}, and the deployment has no evidence \
+                     that it meets it",
                     g.spec.accuracy_metric,
                     g.spec.error.evaluate(),
                     g.spec.failure_probability.evaluate(),
                 ),
                 None => format!(
-                    "q{query}: the output has no guarantee, and the deployment has no evidence \
-                 that it meets target {target:?}"
+                    "q{query}: the output has no guarantee, and the deployment has no \
+                     evidence that it meets target {target:?}"
                 ),
             },
         ));
@@ -4249,6 +4248,87 @@ mod tests {
         }
     }
 
+    /// Deployment evidence of a large absolute error (1.0) for every output.
+    struct EveryOutputMeasuredLoose;
+
+    impl AccuracyEvidenceProvider for EveryOutputMeasuredLoose {
+        fn output_guarantee(&self, _output: &QueryRoot) -> Option<AccuracyGuarantee> {
+            Some(AccuracyGuarantee::new(
+                AccuracyMetric::ABSOLUTE,
+                BoundExpr::Constant { value: 1.0 },
+                ProbabilityExpr::Constant { value: 1e-6 },
+                vec![],
+            ))
+        }
+    }
+
+    /// A model that accepts every output that has no guarantee.
+    struct AcceptsEveryOutputWithoutGuarantee;
+
+    impl AccuracyModel for AcceptsEveryOutputWithoutGuarantee {
+        fn satisfies(&self, guarantee: &AccuracyGuarantee, target: &AccuracyTarget) -> bool {
+            BuiltinAccuracyModel.satisfies(guarantee, target)
+        }
+
+        fn accepts_output_without_guarantee(
+            &self,
+            _output: &QueryRoot,
+            _target: &AccuracyTarget,
+        ) -> bool {
+            true
+        }
+    }
+
+    /// The model's hook decides only when there is no guarantee at all: an
+    /// output with no theoretical guarantee whose evidence guarantee misses
+    /// the target is rejected as `AccuracyTargetMissed`, though the hook
+    /// would accept it. Without that evidence the hook accepts it.
+    #[test]
+    fn a_missed_evidence_guarantee_is_not_overridden_by_the_hook() {
+        let target = AccuracyTarget::max_error(0.01).failure_probability(0.001);
+        let candidates = all_candidates(
+            "quantile_over_time(0.99, m[1m]) - quantile_over_time(0.5, m[1m])",
+            &target,
+        );
+        let summaries: Vec<_> = candidates
+            .iter()
+            .filter(|c| has_summary_estimate(c))
+            .map(|c| c.id.as_str())
+            .collect();
+        assert!(!summaries.is_empty());
+        let select = |evidence: &dyn AccuracyEvidenceProvider| {
+            select_plan(
+                &candidates,
+                &[entry_with(
+                    target.clone().accuracy_metric(AccuracyMetric::ABSOLUTE),
+                    repeating(10_000),
+                )],
+                &data(),
+                PlanningModels::builtin()
+                    .with_accuracy(&AcceptsEveryOutputWithoutGuarantee)
+                    .with_evidence(evidence),
+            )
+            .unwrap()
+        };
+        let loose = select(&EveryOutputMeasuredLoose);
+        for id in &summaries {
+            let rejection = loose.rejected.iter().find(|r| r.id == *id).unwrap();
+            assert_eq!(
+                rejection.cause,
+                RejectionCause::AccuracyTargetMissed,
+                "{}",
+                rejection.reason
+            );
+        }
+        let no_evidence = select(&NoAccuracyEvidence);
+        for id in &summaries {
+            assert!(
+                no_evidence.accepted[*id][0].accepted_without_guarantee,
+                "{id}"
+            );
+        }
+    }
+
     /// `candidates` with every state node (a summary build or an exact
     /// aggregate) kept at ingestion time, so selection charges its kept
     /// bytes every second (a `MatAtQuery` state is held only for one

crates/planner/tests/deployment_accuracy_model.rs

@@ -255,3 +255,50 @@ async fn the_deployment_accuracy_model_decides_outputs_without_a_guarantee() {
     assert!(unguaranteed.contains(&accepting_selection.selected));
     assert!(reads_sketch(&accepting.plans[0].root));
 }
+
+/// The hook decides only outputs with no guarantee: a candidate whose
+/// guarantee misses its target under the built-in models (a relative bound
+/// for a rank target) is still rejected for that under the accepting model.
+#[tokio::test]
+async fn the_hook_does_not_override_a_guarantee_that_misses() {
+    // The user's workload: one p99 query with a 1% rank-error target.
+    let workload = rank_target_workload("quantile_over_time(0.99, lat[5m])");
+    let promql = || FrontendInput::Promql { histograms: None };
+    // The candidates whose guarantee misses the target under the built-in models.
+    let builtin = e2e_plan(UserInput::new(
+        &workload,
+        promql(),
+        PlanningModels::builtin(),
+    ))
+    .await
+    .expect("plans with the built-in models");
+    let missed: BTreeSet<String> = builtin
+        .selection
+        .as_ref()
+        .unwrap()
+        .rejected
+        .iter()
+        .filter(|r| r.cause == RejectionCause::AccuracyTargetMissed)
+        .map(|r| r.id.clone())
+        .collect();
+    assert!(!missed.is_empty());
+    // The same workload with the model that accepts every output without a guarantee.
+    let accepting_model = AcceptsOutputsWithoutGuarantee;
+    let accepting = e2e_plan(UserInput::new(
+        &workload,
+        promql(),
+        PlanningModels::builtin().with_accuracy(&accepting_model),
+    ))
+    .await
+    .expect("plans with the accepting model");
+    let accepting_selection = accepting.selection.as_ref().unwrap();
+    for id in &missed {
+        assert!(
+            accepting_selection
+                .rejected
+                .iter()
+                .any(|r| &r.id == id && r.cause == RejectionCause::AccuracyTargetMissed),
+            "{id}"
+        );
+    }
+}