Skip to content

Commit 4f65ff1

Browse files
zzylolclaude
andcommitted
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 <noreply@anthropic.com>
1 parent e34c85c commit 4f65ff1

3 files changed

Lines changed: 84 additions & 0 deletions

File tree

‎crates/logical-optimizer/src/accuracy/estimators/mod.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,4 +253,7 @@ impl AccuracyModel for EstimatorAccuracy<'_> {
253253
fn satisfies(&self, guarantee: &ResultGuarantee, target: &AccuracyTarget) -> bool {
254254
self.base.satisfies(guarantee, target)
255255
}
256+
fn answers(&self, statistic: &SketchStatistic, guarantee: &ResultGuarantee) -> bool {
257+
self.base.answers(statistic, guarantee)
258+
}
256259
}

‎crates/logical-optimizer/src/accuracy/mod.rs‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,34 @@ pub trait AccuracyModel {
6868
/// Compare the dimensions requested by `target`. Unknown required
6969
/// dimensions fail; selection separately excludes missing accuracy evidence.
7070
fn satisfies(&self, guarantee: &ResultGuarantee, target: &AccuracyTarget) -> bool;
71+
72+
/// Whether `guarantee` bounds the error a target on `statistic` is
73+
/// stated in, so that [`Self::satisfies`] compares like with like. An
74+
/// exact guarantee answers every statistic; otherwise the metric must be
75+
/// one the statistic's ε is measured in. A deployment with a registered
76+
/// cross-metric conversion overrides this.
77+
fn answers(&self, statistic: &SketchStatistic, guarantee: &ResultGuarantee) -> bool {
78+
use ErrorMetric::*;
79+
guarantee.is_exact()
80+
|| match statistic {
81+
SketchStatistic::Quantile { .. } => {
82+
matches!(guarantee.metric, Rank | RelativeValue)
83+
}
84+
SketchStatistic::Cardinality => {
85+
matches!(guarantee.metric, Cardinality | RelativeValue)
86+
}
87+
SketchStatistic::FrequencyL2 | SketchStatistic::FrequencyEntropy => {
88+
guarantee.metric == RelativeValue
89+
}
90+
SketchStatistic::PointCount { .. } => {
91+
matches!(guarantee.metric, Frequency | L2Frequency)
92+
}
93+
// A top-k target bounds the item scores, as Pass 1 checks.
94+
SketchStatistic::TopK { .. } => {
95+
matches!(guarantee.metric, Frequency | L2Frequency | TopKMembership)
96+
}
97+
}
98+
}
7199
}
72100

73101
/// The built-in estimator and composition models, with conservative target checks.
@@ -152,4 +180,24 @@ mod tests {
152180
DefaultAccuracyModel.satisfies(&ResultGuarantee::exact("x"), &AccuracyTarget::Exact)
153181
);
154182
}
183+
184+
/// A bound in another statistic's metric does not answer a target:
185+
/// Count-Min's L1 frequency bound says nothing about a distinct count.
186+
#[test]
187+
fn answers_requires_the_statistics_metric() {
188+
let frequency = ResultGuarantee {
189+
metric: ErrorMetric::Frequency,
190+
..abs(0.01, 0.01)
191+
};
192+
let count = SketchStatistic::PointCount {
193+
key: asap_types::ir::scalar::ColumnRef::SampleValue,
194+
value: None,
195+
};
196+
assert!(DefaultAccuracyModel.answers(&count, &frequency));
197+
assert!(!DefaultAccuracyModel.answers(&SketchStatistic::Cardinality, &frequency));
198+
assert!(!DefaultAccuracyModel.answers(&SketchStatistic::Quantile { q: 0.5 }, &frequency));
199+
assert!(!DefaultAccuracyModel.answers(&SketchStatistic::FrequencyL2, &abs(0.01, 0.01)));
200+
assert!(DefaultAccuracyModel
201+
.answers(&SketchStatistic::Cardinality, &ResultGuarantee::exact("x")));
202+
}
155203
}

‎crates/plan-selection/src/lib.rs‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,6 +1275,12 @@ fn build_violation(
12751275
let Some(guarantee) = models.accuracy.local_guarantee(family, statistic) else {
12761276
return Some(format!("no accuracy model for {name}; target {target:?}"));
12771277
};
1278+
if !models.accuracy.answers(statistic, &guarantee) {
1279+
return Some(format!(
1280+
"{name} guarantees a {:?} bound, which does not bound {statistic:?}; target {target:?}",
1281+
guarantee.metric
1282+
));
1283+
}
12781284
if !models.accuracy.satisfies(&guarantee, target) {
12791285
return Some(format!(
12801286
"{name} guarantees bound {:?}, failure probability {:?}, which misses target \
@@ -2520,6 +2526,33 @@ mod tests {
25202526
}
25212527
}
25222528

2529+
/// A guarantee in another statistic's metric does not satisfy a target:
2530+
/// CountSketch's L2 frequency bound answers the top-k scores it was
2531+
/// built for, but not a distinct count, however loose the target.
2532+
#[test]
2533+
fn a_bound_in_another_metric_misses_the_target() {
2534+
let candidates = candidates();
2535+
let build = OperatorNode::reachable(&candidates[2].roots[0])
2536+
.into_iter()
2537+
.find(|node| {
2538+
matches!(&node.operator, Operator::ASAP(ASAPOp::SummaryAgg { family: FieldDataType::Sketch(kind, _), .. })
2539+
if *kind.algorithm() == SketchAlgorithm::CountSketchWithHeap)
2540+
})
2541+
.expect("a CountSketch build");
2542+
let loose = AccuracyTarget::EpsilonDelta {
2543+
epsilon: 0.5,
2544+
delta: 0.5,
2545+
};
2546+
let models = PlanningModels::builtin();
2547+
assert_eq!(
2548+
build_violation(&build, &SketchStatistic::TopK { k: 10 }, &loose, &models),
2549+
None
2550+
);
2551+
let reason = build_violation(&build, &SketchStatistic::Cardinality, &loose, &models)
2552+
.expect("an L2 frequency bound does not bound a distinct count");
2553+
assert!(reason.contains("does not bound Cardinality"), "{reason}");
2554+
}
2555+
25232556
/// A candidate that cannot be checked is rejected with its reason; the
25242557
/// others are still selected among.
25252558
#[test]

0 commit comments

Comments
 (0)