Skip to content

Commit 52b4003

Browse files
zzylolclaude
andcommitted
feat(ir): reject summary merges of heap-based sketches
A merged top-k heap can miss an item that is heavy in only one input, so CmsWithHeap, CountSketchWithHeap and UnivMon states do not merge. Also correct the merge_disjoint doc: SummaryMerge checks schema, update, reduction and heap families, not accuracy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent d6d9f5b commit 52b4003

3 files changed

Lines changed: 61 additions & 10 deletions

File tree

‎crates/types/src/ir/asap.rs‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use super::summary_coverage::{CoverageError, SummaryCoverage};
1010
use crate::ir::operator_properties::Reduction;
1111
use crate::ir::SchemaDerivationError;
1212
use crate::post_asap::maintained_population::{MaintainedPopulation, PopulationStatistic};
13-
use crate::post_asap::sketch::{GroupingStrategy, SketchStatistic, SummaryUpdate};
13+
use crate::post_asap::sketch::{GroupingStrategy, SketchAlgorithm, SketchStatistic, SummaryUpdate};
1414
use crate::pre_asap::schema::{ColumnId, DataType, Field, FieldDataType, Schema};
1515

1616
/// Why an ASAP operator cannot be used yet.
@@ -42,7 +42,9 @@ pub enum ASAPOp<C = Rc<OperatorNode>> {
4242
},
4343
/// Read an exact accumulator's state as its finalized value: the
4444
/// maintenance-to-read boundary before query-time operators.
45-
FinalizeExactAccumulator { child: C },
45+
FinalizeExactAccumulator {
46+
child: C,
47+
},
4648
/// Maintain the full declared population, including membership changes.
4749
MaintainPopulation {
4850
child: C,
@@ -54,7 +56,9 @@ pub enum ASAPOp<C = Rc<OperatorNode>> {
5456
evaluation: PopulationStatistic,
5557
},
5658
/// Merge compatible partial states for the same grouping and family.
57-
SummaryMerge { children: Vec<C> },
59+
SummaryMerge {
60+
children: Vec<C>,
61+
},
5862
// ── Reserved: migrated but unimplemented ──
5963
SummarySubtract {
6064
left: C,
@@ -498,6 +502,23 @@ impl ASAPOp {
498502
));
499503
}
500504
}
505+
// A heap of top-k candidates does not merge exactly: an item heavy
506+
// in only one input can be missing from the merged heap.
507+
if first.schema.fields.iter().any(|field| {
508+
matches!(
509+
&field.dtype,
510+
FieldDataType::Sketch(kind, _) if matches!(
511+
kind.algorithm(),
512+
SketchAlgorithm::CmsWithHeap
513+
| SketchAlgorithm::CountSketchWithHeap
514+
| SketchAlgorithm::UnivMon
515+
)
516+
)
517+
}) {
518+
return Err(SchemaDerivationError::InvalidScalarSignature(
519+
"summary merge does not support heap-based sketches".into(),
520+
));
521+
}
501522
// Coverage records only time and population; what each state
502523
// summarizes and how it is grouped come from the producers.
503524
let update = first.summary_update();

‎crates/types/src/ir/summary_coverage.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,10 @@ impl SummaryCoverage {
7575

7676
/// Every observation in a region is assumed to contribute once to the state.
7777
/// Compose once-per-observation summaries only when their joint regions are
78-
/// provably disjoint. Update/reduction compatibility, family merge capability
79-
/// and accuracy are checked by `SummaryMerge`, not here.
78+
/// provably disjoint. `SummaryMerge` checks the rest: equal schemas (so the
79+
/// same family and parameters), equal update and reduction, and no
80+
/// heap-based family. Accuracy of the merged state is not assessed; its
81+
/// `guarantee` is `None`.
8082
pub fn merge_disjoint(inputs: &[Self]) -> Result<Self, CoverageError> {
8183
let first = inputs.first().ok_or(CoverageError::EmptyMerge)?;
8284
let mut merged = first.clone();

‎crates/types/tests/summary_merge_structure.rs‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,13 @@ use asap_types::{
77
};
88
use std::rc::Rc;
99
fn state(k: u32) -> Rc<OperatorNode> {
10+
state_with(FieldDataType::Sketch(
11+
SketchKind::new(SketchAlgorithm::Kll, SketchParams::Kll { k }),
12+
Default::default(),
13+
))
14+
}
15+
16+
fn state_with(family: FieldDataType) -> Rc<OperatorNode> {
1017
let scan = OperatorNode::new_shared(Operator::NonASAP(NonASAPOp::Scan {
1118
source: Source::Table {
1219
table_ref: "latencies".into(),
@@ -17,10 +24,7 @@ fn state(k: u32) -> Rc<OperatorNode> {
1724
.unwrap();
1825
let summary = OperatorNode::new(Operator::ASAP(ASAPOp::SummaryAgg {
1926
child: scan,
20-
family: FieldDataType::Sketch(
21-
SketchKind::new(SketchAlgorithm::Kll, SketchParams::Kll { k }),
22-
Default::default(),
23-
),
27+
family,
2428
input: SummaryUpdate::column(ColumnRef::SampleValue),
2529
reduction: Reduction::by(vec![]),
2630
grouping: GroupingStrategy::default(),
@@ -70,11 +74,35 @@ fn incompatible_merge_inputs_fail() {
7074
}
7175

7276
fn shifted_state(k: u32, start: i64, end: i64) -> Rc<OperatorNode> {
73-
let mut node = (*state(k)).clone();
77+
shifted(&state(k), start, end)
78+
}
79+
80+
fn shifted(state: &Rc<OperatorNode>, start: i64, end: i64) -> Rc<OperatorNode> {
81+
let mut node = (**state).clone();
7482
let region = &mut node.coverage.as_mut().unwrap().regions[0];
7583
region.time_ms = Some(start..end);
7684
Rc::new(node)
7785
}
86+
87+
/// A heap of top-k candidates does not merge exactly, even over disjoint panes.
88+
#[test]
89+
fn heap_based_sketches_do_not_merge() {
90+
let heap = state_with(FieldDataType::Sketch(
91+
SketchKind::new(
92+
SketchAlgorithm::CmsWithHeap,
93+
SketchParams::CmsWithHeap {
94+
width: 1024,
95+
depth: 4,
96+
heap_size: 10,
97+
},
98+
),
99+
Default::default(),
100+
));
101+
let result = OperatorNode::new_shared(Operator::ASAP(ASAPOp::SummaryMerge {
102+
children: vec![Rc::clone(&heap), shifted(&heap, 1, 2)],
103+
}));
104+
assert!(result.is_err());
105+
}
78106
/// Schema equality cannot authorize overlapping or unknown observation coverage.
79107
#[test]
80108
fn unsafe_coverage_merge_is_rejected() {

0 commit comments

Comments
 (0)