From 4a49398f1aa9413abcab0efbc1e65989add550bf Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:52:58 -0700 Subject: [PATCH] fix(core): a Summary can be built from an engine's parts, and keys stop cloning values (improvise-qddo) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/source.md | 6 ++ context/test-evidence/core.md | 17 ++++ crates/improvise-core/src/model/source.rs | 106 +++++++++++++++++++--- 3 files changed, 117 insertions(+), 12 deletions(-) diff --git a/context/code/source.md b/context/code/source.md index cffcbe47..22ccb6c1 100644 --- a/context/code/source.md +++ b/context/code/source.md @@ -70,3 +70,9 @@ implementation, written as a query engine with no index would answer, sharing on `DataStore`. `the_store_and_a_scanned_table_answer_every_select_alike` asks both every select over a fixture (every subset of every stored key as a base, with no span, one span of each of six item lists, and two), at both widths and for all five functions: over 5,000 triples. + +## Building a `Summary` from an engine's answer + +`Summary::from_parts(Numbers::from_parts(count, exact_sum, least, greatest), Agreement)`. `Agreement` +is what the non-number values hold: `None`, `One(v)` or `Mixed`. `from_parts` drops extremes that +contradict the count or are NaN, so a source cannot hand the evaluator a state `add` could not reach. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index c6fac22a..abbd932d 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -2344,3 +2344,20 @@ the member records of an aggregate over an expression A select with `exact` and no span is the cell, not the cells containing the pairs. The store's `spanned_values` is never called for it (the evaluator reads an exact key through `value_at`), so this is the contract a second source implements. Rival: `exact` ignored when there is no span. + +### a_summary_built_from_an_engines_parts_reduces_as_one_built_from_the_values + +Review of #356 (Codex): an engine outside core could not build a `Summary` without replaying rows. The +parts (count, exact sum, extremes, agreement of the non-numbers) are computed in the test without +`Summary::of`, for the 585 sequences of the reference test. Rivals: the greatest taken from the least; +a NaN allowed as an extreme. Each fails it. + +### parts_that_contradict_themselves_are_made_consistent + +Extremes with no numbers, and a NaN as an extreme, are dropped by `Numbers::from_parts`. + +### keys_of_a_store_are_its_cells_keys_without_the_values + +Review of #356 (Codex): `DataStore::keys` went through `cells` and cloned every value. It reads keys +only now; the test pins the keys and their order against `cells`. No clone-count test: a value clone is +not observable from outside, so the equality is all that can be asserted. diff --git a/crates/improvise-core/src/model/source.rs b/crates/improvise-core/src/model/source.rs index f25fea70..cf78800c 100644 --- a/crates/improvise-core/src/model/source.rs +++ b/crates/improvise-core/src/model/source.rs @@ -51,6 +51,23 @@ impl Numbers { numbers } + /// Numbers from what an engine computed: how many, their exact sum and the least and + /// greatest that are not NaN. Extremes that contradict the count or are NaN are dropped. + pub fn from_parts( + count: u64, + sum: ExactSum, + least: Option, + greatest: Option, + ) -> Self { + let held = |x: Option| x.filter(|x| count > 0 && !x.is_nan()); + Numbers { + count, + sum, + least: held(least), + greatest: held(greatest), + } + } + pub fn add(&mut self, x: f64) { self.count += 1; self.sum.add(x, 1); @@ -104,9 +121,9 @@ fn merged(a: Option, b: Option, pick: fn(f64, f64) -> f64) -> Option Self { + Summary { numbers, others } + } + pub fn add(&mut self, value: &CellValue) { match value.as_f64() { Some(x) => self.numbers.add(x), @@ -137,18 +159,18 @@ impl Summary { fn add_other(&mut self, value: &CellValue) { self.others = match std::mem::take(&mut self.others) { - Others::None => Others::One(value.clone()), - Others::One(held) if held == *value => Others::One(held), - _ => Others::Mixed, + Agreement::None => Agreement::One(value.clone()), + Agreement::One(held) if held == *value => Agreement::One(held), + _ => Agreement::Mixed, }; } pub fn merge(&mut self, other: &Summary) { self.numbers.merge(&other.numbers); match &other.others { - Others::None => {} - Others::One(v) => self.add_other(v), - Others::Mixed => self.others = Others::Mixed, + Agreement::None => {} + Agreement::One(v) => self.add_other(v), + Agreement::Mixed => self.others = Agreement::Mixed, } } @@ -159,8 +181,8 @@ impl Summary { pub fn reduce(&self, func: &AggFunc) -> Option { match (&self.others, self.numbers.count()) { - (Others::None, _) => self.numbers.reduce(func).map(CellValue::Number), - (Others::One(only), 0) => Some(only.clone()), + (Agreement::None, _) => self.numbers.reduce(func).map(CellValue::Number), + (Agreement::One(only), 0) => Some(only.clone()), _ => Some(CellValue::Error("mixed".to_string())), } } @@ -206,7 +228,10 @@ impl CellSource for DataStore { &'a self, partial: &'a [(String, String)], ) -> Box + 'a> { - Box::new(self.cells(partial).map(|(key, _)| key)) + match partial.is_empty() { + true => Box::new(self.iter_cells().map(|(key, _)| key)), + false => Box::new(self.matching_cells(partial).into_iter().map(|(key, _)| key)), + } } fn cells<'a>( @@ -555,4 +580,61 @@ mod tests { "no cell is keyed by R=E alone" ); } + + /// The parts an engine would compute for `values`, found without `Summary::of`. + fn parts_of(values: &[CellValue]) -> Summary { + let numbers: Vec = values.iter().filter_map(|v| v.as_f64()).collect(); + let mut sum = ExactSum::new(); + numbers.iter().for_each(|x| sum.add(*x, 1)); + let finite = || numbers.iter().copied().filter(|x| !x.is_nan()); + let least = finite().min_by(f64::total_cmp); + let greatest = finite().max_by(f64::total_cmp); + let rest: Vec<&CellValue> = values.iter().filter(|v| v.as_f64().is_none()).collect(); + let agreement = match rest.split_first() { + None => Agreement::None, + Some((first, others)) if others.iter().all(|v| v == first) => { + Agreement::One((*first).clone()) + } + Some(_) => Agreement::Mixed, + }; + let parts = Numbers::from_parts(numbers.len() as u64, sum, least, greatest); + Summary::from_parts(parts, agreement) + } + + #[test] + fn a_summary_built_from_an_engines_parts_reduces_as_one_built_from_the_values() { + for values in sequences(3) { + let refs: Vec<&CellValue> = values.iter().collect(); + for func in &FUNCS { + let want = reference(&refs, func); + let got = parts_of(&values).reduce(func); + assert!( + same(&want, &got), + "{func:?} of {values:?}: {want:?} != {got:?}" + ); + } + } + } + + #[test] + fn parts_that_contradict_themselves_are_made_consistent() { + let none = Numbers::from_parts(0, ExactSum::new(), Some(1.0), Some(f64::NAN)); + assert_eq!(none.reduce(&AggFunc::Min), None, "no numbers, no extremes"); + let nan = Numbers::from_parts(2, ExactSum::new(), Some(f64::NAN), None); + let min = nan.reduce(&AggFunc::Min).unwrap(); + assert!(min.is_nan(), "a NaN is never an extreme"); + } + + #[test] + fn keys_of_a_store_are_its_cells_keys_without_the_values() { + let rows = rows(); + let store = store_of(&rows); + let all: Vec = store.keys(&[]).collect(); + assert_eq!(all, rows.iter().map(|(k, _)| k.clone()).collect::>()); + let east = vec![pair("R", "E")]; + let under: Vec = store.keys(&east).collect(); + let want: Vec = store.cells(&east).map(|c| c.0).collect(); + assert_eq!(under, want); + assert!(!under.is_empty()); + } }