From 8ba4bd0a6cfee1698cd05bb1403d2a3fbc2c9023 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 19:30:22 -0700 Subject: [PATCH] fix(core): summaries are tested for exact merges, impossible parts are refused, and the bypass list is complete (improvise-qddo) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/source.md | 15 ++- context/test-evidence/core.md | 32 +++++- crates/improvise-core/src/model/source.rs | 131 ++++++++++++++++++---- 3 files changed, 145 insertions(+), 33 deletions(-) diff --git a/context/code/source.md b/context/code/source.md index 22ccb6c1..10a91cc5 100644 --- a/context/code/source.md +++ b/context/code/source.md @@ -60,8 +60,11 @@ either returns the numbers (and the adapter sums them) or accepts a last-bit dif `Model::source()` returns the `&dyn CellSource` that the evaluator, the stems of `recompute_formulas` and the records of `GridLayout` read through. Today it is always the owned `DataStore`; the point of -routing the reads is that PR 3 (improvise-rnak) changes only this accessor. Reads that bypass it -(`model.data.*` in other crates, `Eager::occupancy`, `Shapes::of`) are the ones later steps move. +routing the reads is that PR 3 (improvise-rnak) changes only this accessor. It is not the only path: `Model::data` is `pub`, and `Model::get_cell` reads the owned +store directly. Both are the facade of a document that owns its cells; their callers are persistence, +the import wizard and `improvise-command` (`ui/effect.rs`, the `old` value of a rename at line 890, is +the one production read of `get_cell` outside those), and a model over a source has no cells there. +`Eager::occupancy` and `Shapes::of` were the other readers; later steps moved them. ## `TableSource` @@ -73,6 +76,8 @@ 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. +`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`. Both constructors return a `Result` +and **refuse** a state no sequence of values could produce (extremes with no numbers, a NaN extreme, +least above greatest, one extreme without the other, a number among the "others"), because repairing +it would hide an engine's bug. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index abbd932d..0791c7cb 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -2314,6 +2314,11 @@ lost). The first two also fail `aggregating_differing_text_values_is_an_explicit ### model::source::tests::a_merge_of_summaries_is_the_summary_of_the_union +Review of #356 (Opus): the pool gained `1e16`, `1.0`, `-1e16`, `0.1` and `0.2`. A merge rounding each part's +sum before pooling, keeping the zero sign, changed 514 production answers while the workspace passed; +that rival now fails this test, `numbers_pool_what_the_formula_aggregate_pools` (a NaN-free split now, +compared as bits), `summaries_built_from_parts_merge_as_the_union_does` and the fixture test below, by name. + A read of several parts is a merge. The test merges every ordered pair of sequences of up to two values and compares with the reference of the concatenation. Rivals: the least not merged, the greatest merged with the other side's least, the zeros unordered. Each fails it. @@ -2335,7 +2340,7 @@ span, and the store ignores `exact` for a spanned one. The first also fails ### model::source::tests::the_store_and_a_scanned_table_list_the_same_cells `keys` and `cells` under every base of the fixture, and `value_at` for every row and an absent key. -Rival: `DataStore::keys` ignores its partial. It fails here and in five evaluator tests that read +Rival: `DataStore::keys` ignores its partial. It fails here and in seven evaluator tests that read the member records of an aggregate over an expression (`route_then_aggregate_tests::a_routing_formula_at_a_record_is_the_rule_itself` among them). @@ -2348,13 +2353,28 @@ this is the contract a second source implements. Rival: `exact` ignored when the ### 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 are computed in the test without `Summary::of`, for every sequence of up to three values of the +pool. No rival of the first version failed this test alone (its filter was untested); see below. -### parts_that_contradict_themselves_are_made_consistent +### summaries_built_from_parts_merge_as_the_union_does -Extremes with no numbers, and a NaN as an extreme, are dropped by `Numbers::from_parts`. +A summary built from parts, merged with one built from values and with one built from parts, equals the +reference of the union, for every pair of sequences of up to two values. This is where a bad extreme +would matter, since merge reads the extremes. + +### parts_no_sequence_of_values_could_produce_are_refused + +Review of #356 (Opus, Codex): `from_parts` repaired some impossible states and accepted others +(`One(Number)`, least above greatest). It refuses all of them with a `BadParts` naming the reason. +The earlier entry's claim that a NaN-extreme rival failed `parts_that_contradict_themselves_...` was +false: that test passed with the filter removed. Rivals now, each failing this test alone: NaN extreme +allowed, least above greatest allowed, extremes with no numbers allowed, a number accepted among the +others. + +### the_exact_sum_of_a_pool_of_cancelling_values_is_not_the_sum_of_rounded_parts + +An exactness test needs inputs whose float sum is inexact and no NaN. `1e16 + 1.0 - 1e16` rounded part +by part is `0.0`; pooled exactly it is `1.0`. This pins the fixture rather than the code. ### keys_of_a_store_are_its_cells_keys_without_the_values diff --git a/crates/improvise-core/src/model/source.rs b/crates/improvise-core/src/model/source.rs index cf78800c..384e9ca0 100644 --- a/crates/improvise-core/src/model/source.rs +++ b/crates/improvise-core/src/model/source.rs @@ -52,20 +52,28 @@ impl 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. + /// greatest that are not NaN. A state `add` cannot reach is refused, not repaired. 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 { + ) -> Result { + let bad = |why| Err(BadParts(why)); + match (least, greatest) { + (None, None) => {} + (Some(_), Some(_)) if count == 0 => return bad("extremes with no numbers"), + (Some(l), Some(g)) if l.is_nan() || g.is_nan() => return bad("a NaN extreme"), + (Some(l), Some(g)) if l.total_cmp(&g).is_gt() => return bad("least above greatest"), + (Some(_), Some(_)) => {} + _ => return bad("one extreme without the other"), + } + Ok(Numbers { count, sum, - least: held(least), - greatest: held(greatest), - } + least, + greatest, + }) } pub fn add(&mut self, x: f64) { @@ -121,6 +129,10 @@ fn merged(a: Option, b: Option, pick: fn(f64, f64) -> f64) -> Option Self { - Summary { numbers, others } + pub fn from_parts(numbers: Numbers, others: Agreement) -> Result { + match &others { + Agreement::One(v) if v.as_f64().is_some() => Err(BadParts("a number among the others")), + _ => Ok(Summary { numbers, others }), + } } pub fn add(&mut self, value: &CellValue) { @@ -324,6 +339,11 @@ mod tests { CellValue::Number(0.0), CellValue::Number(f64::NAN), CellValue::Number(f64::INFINITY), + CellValue::Number(1e16), + CellValue::Number(1.0), + CellValue::Number(-1e16), + CellValue::Number(0.1), + CellValue::Number(0.2), text("a"), text("b"), CellValue::Error("e".into()), @@ -416,23 +436,41 @@ mod tests { #[test] fn numbers_pool_what_the_formula_aggregate_pools() { - let xs = [1.0, -0.0, f64::NAN, 3.0, 0.0]; + let xs = [1e16, 1.0, -1e16, 0.1, 0.2, -0.0, 0.0, 3.0]; for split in 0..=xs.len() { let mut pooled = Numbers::of(xs[..split].iter().copied()); pooled.merge(&Numbers::of(xs[split..].iter().copied())); let whole = Numbers::of(xs); for func in &FUNCS { let (a, b) = (pooled.reduce(func), whole.reduce(func)); - let equal = a.map(f64::to_bits) == b.map(f64::to_bits) - || a.zip(b).is_some_and(|(x, y)| x.is_nan() && y.is_nan()); - assert!(equal, "{func:?} split at {split}: {a:?} != {b:?}"); + assert_eq!( + a.map(f64::to_bits), + b.map(f64::to_bits), + "{func:?} split at {split}" + ); } - assert_eq!(pooled.total().to_bits(), whole.total().to_bits()); + assert_eq!( + pooled.total().to_bits(), + whole.total().to_bits(), + "split at {split}" + ); } assert_eq!(Numbers::default().total().to_bits(), 0.0f64.to_bits()); assert_eq!(Numbers::default().reduce(&AggFunc::Sum), None); } + #[test] + fn the_exact_sum_of_a_pool_of_cancelling_values_is_not_the_sum_of_rounded_parts() { + let rounded = [1e16, 1.0].iter().sum::() + [-1e16].iter().sum::(); + let mut pooled = Numbers::of([1e16, 1.0]); + pooled.merge(&Numbers::of([-1e16])); + assert_eq!( + rounded, 0.0, + "the fixture is inexact: summing rounded parts loses the 1" + ); + assert_eq!(pooled.total(), 1.0); + } + fn pair(c: &str, i: &str) -> (String, String) { (c.to_string(), i.to_string()) } @@ -597,8 +635,8 @@ mod tests { } Some(_) => Agreement::Mixed, }; - let parts = Numbers::from_parts(numbers.len() as u64, sum, least, greatest); - Summary::from_parts(parts, agreement) + let parts = Numbers::from_parts(numbers.len() as u64, sum, least, greatest).unwrap(); + Summary::from_parts(parts, agreement).unwrap() } #[test] @@ -617,12 +655,61 @@ mod tests { } #[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"); + fn summaries_built_from_parts_merge_as_the_union_does() { + let all = sequences(2); + for (a, b) in all.iter().flat_map(|a| all.iter().map(move |b| (a, b))) { + let mut merged = parts_of(a); + merged.merge(&Summary::of(b)); + let mut both = parts_of(a); + both.merge(&parts_of(b)); + let union: Vec<&CellValue> = a.iter().chain(b).collect(); + for func in &FUNCS { + let want = reference(&union, func); + assert!( + same(&want, &merged.reduce(func)), + "{func:?} of {a:?} + {b:?}" + ); + assert!( + same(&want, &both.reduce(func)), + "{func:?} of {a:?} + {b:?}, both from parts" + ); + } + } + } + + #[test] + fn parts_no_sequence_of_values_could_produce_are_refused() { + let sum = ExactSum::new; + let refused = |r: Result| r.expect_err("must be refused").0; + assert_eq!( + refused(Numbers::from_parts(0, sum(), Some(1.0), Some(1.0))), + "extremes with no numbers" + ); + assert_eq!( + refused(Numbers::from_parts(2, sum(), Some(f64::NAN), Some(1.0))), + "a NaN extreme" + ); + assert_eq!( + refused(Numbers::from_parts(2, sum(), Some(2.0), Some(1.0))), + "least above greatest" + ); + assert_eq!( + refused(Numbers::from_parts(2, sum(), Some(0.0), Some(-0.0))), + "least above greatest" + ); + assert_eq!( + refused(Numbers::from_parts(2, sum(), Some(1.0), None)), + "one extreme without the other" + ); + let ok = Numbers::from_parts(2, sum(), None, None).unwrap(); + assert!( + ok.reduce(&AggFunc::Min).unwrap().is_nan(), + "all NaN: no extremes, a count" + ); + let number = Agreement::One(CellValue::Number(1.0)); + let bad = Summary::from_parts(ok.clone(), number).expect_err("a number is not an other"); + assert_eq!(bad.0, "a number among the others"); + assert!(Summary::from_parts(ok, Agreement::One(CellValue::Text("t".into()))).is_ok()); } #[test]