From e30d21a6421bd4af90b65348f1587ca575ea31fa Mon Sep 17 00:00:00 2001 From: zzylol <50204836+zzylol@users.noreply.github.com> Date: Mon, 5 Oct 2026 00:38:49 +0000 Subject: [PATCH] fix(plan-selection): reject a guarantee in another statistic's metric Stage 3 compared only a summary guarantee's bound and failure probability with the target, so a bound in one metric (Count-Min's L1 frequency error, say) could satisfy a target on another statistic (a distinct count). `AccuracyModel::answers` maps each statistic to the metrics its epsilon is stated in, and `build_violation` checks it before `satisfies`. Co-Authored-By: Claude Opus 5.5 --- .../src/accuracy/estimators/mod.rs | 3 ++ crates/logical-optimizer/src/accuracy/mod.rs | 48 +++++++++++++++++++ crates/plan-selection/src/lib.rs | 33 +++++++++++++ 3 files changed, 84 insertions(+) diff --git a/crates/logical-optimizer/src/accuracy/estimators/mod.rs b/crates/logical-optimizer/src/accuracy/estimators/mod.rs index a41aba11..c3406b83 100644 --- a/crates/logical-optimizer/src/accuracy/estimators/mod.rs +++ b/crates/logical-optimizer/src/accuracy/estimators/mod.rs @@ -253,4 +253,7 @@ impl AccuracyModel for EstimatorAccuracy<'_> { fn satisfies(&self, guarantee: &ResultGuarantee, target: &AccuracyTarget) -> bool { self.base.satisfies(guarantee, target) } + fn answers(&self, statistic: &SketchStatistic, guarantee: &ResultGuarantee) -> bool { + self.base.answers(statistic, guarantee) + } } diff --git a/crates/logical-optimizer/src/accuracy/mod.rs b/crates/logical-optimizer/src/accuracy/mod.rs index 08ec2747..762ab96b 100644 --- a/crates/logical-optimizer/src/accuracy/mod.rs +++ b/crates/logical-optimizer/src/accuracy/mod.rs @@ -68,6 +68,34 @@ pub trait AccuracyModel { /// Compare the dimensions requested by `target`. Unknown required /// dimensions fail; selection separately excludes missing accuracy evidence. fn satisfies(&self, guarantee: &ResultGuarantee, target: &AccuracyTarget) -> bool; + + /// Whether `guarantee` bounds the error a target on `statistic` is + /// stated in, so that [`Self::satisfies`] compares like with like. An + /// exact guarantee answers every statistic; otherwise the metric must be + /// one the statistic's ε is measured in. A deployment with a registered + /// cross-metric conversion overrides this. + fn answers(&self, statistic: &SketchStatistic, guarantee: &ResultGuarantee) -> bool { + use ErrorMetric::*; + guarantee.is_exact() + || match statistic { + SketchStatistic::Quantile { .. } => { + matches!(guarantee.metric, Rank | RelativeValue) + } + SketchStatistic::Cardinality => { + matches!(guarantee.metric, Cardinality | RelativeValue) + } + SketchStatistic::FrequencyL2 | SketchStatistic::FrequencyEntropy => { + guarantee.metric == RelativeValue + } + SketchStatistic::PointCount { .. } => { + matches!(guarantee.metric, Frequency | L2Frequency) + } + // A top-k target bounds the item scores, as Pass 1 checks. + SketchStatistic::TopK { .. } => { + matches!(guarantee.metric, Frequency | L2Frequency | TopKMembership) + } + } + } } /// The built-in estimator and composition models, with conservative target checks. @@ -152,4 +180,24 @@ mod tests { DefaultAccuracyModel.satisfies(&ResultGuarantee::exact("x"), &AccuracyTarget::Exact) ); } + + /// A bound in another statistic's metric does not answer a target: + /// Count-Min's L1 frequency bound says nothing about a distinct count. + #[test] + fn answers_requires_the_statistics_metric() { + let frequency = ResultGuarantee { + metric: ErrorMetric::Frequency, + ..abs(0.01, 0.01) + }; + let count = SketchStatistic::PointCount { + key: asap_types::ir::scalar::ColumnRef::SampleValue, + value: None, + }; + assert!(DefaultAccuracyModel.answers(&count, &frequency)); + assert!(!DefaultAccuracyModel.answers(&SketchStatistic::Cardinality, &frequency)); + assert!(!DefaultAccuracyModel.answers(&SketchStatistic::Quantile { q: 0.5 }, &frequency)); + assert!(!DefaultAccuracyModel.answers(&SketchStatistic::FrequencyL2, &abs(0.01, 0.01))); + assert!(DefaultAccuracyModel + .answers(&SketchStatistic::Cardinality, &ResultGuarantee::exact("x"))); + } } diff --git a/crates/plan-selection/src/lib.rs b/crates/plan-selection/src/lib.rs index df6aa078..a62209ee 100644 --- a/crates/plan-selection/src/lib.rs +++ b/crates/plan-selection/src/lib.rs @@ -1276,6 +1276,12 @@ fn build_violation( let Some(guarantee) = models.accuracy.local_guarantee(family, statistic) else { return Some(format!("no accuracy model for {name}; target {target:?}")); }; + if !models.accuracy.answers(statistic, &guarantee) { + return Some(format!( + "{name} guarantees a {:?} bound, which does not bound {statistic:?}; target {target:?}", + guarantee.metric + )); + } if !models.accuracy.satisfies(&guarantee, target) { return Some(format!( "{name} guarantees bound {:?}, failure probability {:?}, which misses target \ @@ -2515,6 +2521,33 @@ mod tests { } } + /// A guarantee in another statistic's metric does not satisfy a target: + /// CountSketch's L2 frequency bound answers the top-k scores it was + /// built for, but not a distinct count, however loose the target. + #[test] + fn a_bound_in_another_metric_misses_the_target() { + let candidates = candidates(); + let build = OperatorNode::reachable(&candidates[2].roots[0]) + .into_iter() + .find(|node| { + matches!(&node.operator, Operator::ASAP(ASAPOp::SummaryAgg { family: FieldDataType::Sketch(kind, _), .. }) + if *kind.algorithm() == SketchAlgorithm::CountSketchWithHeap) + }) + .expect("a CountSketch build"); + let loose = AccuracyTarget::EpsilonDelta { + epsilon: 0.5, + delta: 0.5, + }; + let models = PlanningModels::builtin(); + assert_eq!( + build_violation(&build, &SketchStatistic::TopK { k: 10 }, &loose, &models), + None + ); + let reason = build_violation(&build, &SketchStatistic::Cardinality, &loose, &models) + .expect("an L2 frequency bound does not bound a distinct count"); + assert!(reason.contains("does not bound Cardinality"), "{reason}"); + } + /// A candidate that cannot be checked is rejected with its reason; the /// others are still selected among. #[test]