From 5fdf50287112d9240c21125144a932dcb35fd372 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 17:55:14 -0700 Subject: [PATCH 1/5] refactor(core): a read-only CellSource trait, DataStore its first implementation (improvise-qddo) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/source.md | 72 +++ context/test-evidence/core.md | 44 ++ crates/improvise-core/src/model/mod.rs | 1 + crates/improvise-core/src/model/source.rs | 558 ++++++++++++++++++++++ crates/improvise-core/src/model/types.rs | 110 ++--- crates/improvise-core/src/view/layout.rs | 23 +- scripts/build-doc-site.mjs | 1 + 7 files changed, 716 insertions(+), 93 deletions(-) create mode 100644 context/code/source.md create mode 100644 crates/improvise-core/src/model/source.rs diff --git a/context/code/source.md b/context/code/source.md new file mode 100644 index 00000000..cffcbe47 --- /dev/null +++ b/context/code/source.md @@ -0,0 +1,72 @@ +# `CellSource`: where cell values are read from + +`crates/improvise-core/src/model/source.rs`. ADR 003 asked for a source trait in core with the +in-RAM `DataStore` as one implementation (improvise-qddo). ADR 009 step 7 (improvise-rnak) asks for +the two live sources that need it. This file is the reasoning; the evidence for each test is in +`context/test-evidence/core.md`. The ADRs are not amended: what follows are the readings taken where +they leave a choice. + +## What is in the trait, and why + +| method | what the evaluator asks | +|---|---| +| `value_at(key)` | the value stored at exactly `key` (an exact read, the filter-mismatch fallback) | +| `summarize(select)` | the summary of the cells a `Select` names: the aggregate, not the rows | +| `keys(partial)` | the keys of the cells containing `partial`: stems, and the records behind an aggregate | +| `cells(partial)` | the same with values: records mode and a drill | + +1. **Aggregation is the interface.** ADR 003 clause 1: `GridLayout` asks for a value, not for the + rows behind it, and the aggregates are SQL's. So the one read the evaluator makes in bulk, + `summarize`, returns a `Summary` and never a list of values. `keys` and `cells` exist because + some reads are of records, not of a fold (the stems a formula is evaluated at; records mode), and + a SQL source answers them with a `SELECT DISTINCT` or a plain select. +2. **Writes are not in the trait.** `DataStore::set` and `remove` are inherent methods. A source that + cannot be written to has no method to fail in, which is ADR 003's "unrepresentable rather than + enforced". The model's own writes (`set_cell`, `clear_cell`) go to the owned store. +3. **The trait has no `Result`.** A source that can fail (a connection) decides at its own boundary + what a failed read is, and `improvise-io` is where that decision belongs. AGENTS.md keeps + `Option` and `Result` at the edges; `Option` is the one the trait uses, for "no such cell". +4. **`Select` is `spanned_values`' arguments.** A fold names a partial key, the covered members of + each group or omitted category it spans, and whether only an exact-width cell counts. The + covered members are decided in RAM from the categories and the view's hidden items, so a source + is handed explicit item lists and never a predicate. A hidden item therefore reaches a source + only as the absence of an item from a list. + +## `Summary` and `Numbers` + +A `Summary` is what a source reports about the values of a read: a `Numbers` (count, an exact sum, +the least and greatest non-NaN, in `total_cmp` order so `-0.0` is below `+0.0`) and whether the rest +(text, errors) agree on one value. It reduces exactly as `aggregate_cell_values` did: numbers by the +function, anything else by unanimity, else `Error("mixed")`, none at all absent. Three properties +carry the design: + +- **It merges.** A read of a collapsed group, a total, or several contexts of a formula aggregate is + the merge of the summaries of its parts, equal to the summary of the union. That is what lets an + engine answer a pooled read without sending rows, and what lets `eval_agg` pool `Numbers` instead + of concatenating `Vec`. The sum is `ExactSum`, so a merge is the same bits in any order. +- **It is order-free.** Unanimity needs the set of non-number values, not their order, so a source + that has no insertion order loses nothing by it. +- **The reference is kept.** `tests::reference` is the old reduction, copied into the test module; + `a_summary_reduces_as_the_values_did` compares every sequence of an eight-value pool, up to three + long, with `Summary` over all five functions. `characterization_survives_the_move`: the existing + evaluator tests (423 in the crate) are unchanged and pass. + +What a real engine must supply is stated here because the trait cannot enforce it: a `Numbers` whose +sum is exact. SQLite's `SUM` is a float sum, so an engine that cannot return an exact accumulator +either returns the numbers (and the adapter sums them) or accepts a last-bit difference, which +`context/code/improvise-incremental.md` ("Step 6") calls a user-visible change. + +## `Model::source()` + +`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. + +## `TableSource` + +A scanned table of rows, behind `cfg(any(test, feature = "test-support"))`. It is the second +implementation, written as a query engine with no index would answer, sharing only `Summary` with +`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. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 56e15d2b..c6fac22a 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -2300,3 +2300,47 @@ key with the whole suite green. It is reached by `reopened_at` (an advance), `refused_here` (a commit) and `settle_declined_editor`. Rival: the arm passes `category`; the test fails on all three assertions, and the display is 36 characters against the key "Region". + +### model::source::tests::a_summary_reduces_as_the_values_did + +improvise-qddo. The reduction moved from `aggregate_cell_values` to `Summary::reduce`; the test keeps +the old function as `reference` and compares all 585 value sequences of up to three values from an +eight-value pool (NaN, both zeros, infinity, two texts, an error) on all five functions. Rivals, each +failing it by name: unanimity ignored (`Others::One` kept for any second value), a number beside a +unanimous text read as unanimous, and `<` for the order of the zeros (the sign of a zero extreme is +lost). The first two also fail `aggregating_differing_text_values_is_an_explicit_error` and +`aggregating_text_among_numbers_is_an_explicit_error`; the zero rival also fails +`eager_math_tests::min_and_max_of_both_zeros_do_not_depend_on_their_order`. + +### model::source::tests::a_merge_of_summaries_is_the_summary_of_the_union + +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. + +### model::source::tests::numbers_pool_what_the_formula_aggregate_pools + +`eval_agg` pools `Numbers` now, not a concatenated `Vec`. Every split of a five-number list +(a NaN and both zeros among them) pooled by merge must equal the whole, by bits for all five +functions and for the total. Rivals: the least not merged, the greatest seeded from the other +side's least. Both fail it. + +### model::source::tests::the_store_and_a_scanned_table_answer_every_select_alike + +`DataStore` and `TableSource` (a scan, sharing only `Summary`) are asked every select over the +fixture, at both widths. Rivals, each failing it: the store ignores `exact` for a select with no +span, and the store ignores `exact` for a spanned one. The first also fails +`an_exact_select_with_no_span_names_one_cell`. + +### 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 +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). + +### model::source::tests::an_exact_select_with_no_span_names_one_cell + +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. diff --git a/crates/improvise-core/src/model/mod.rs b/crates/improvise-core/src/model/mod.rs index f0c5abb7..29895e30 100644 --- a/crates/improvise-core/src/model/mod.rs +++ b/crates/improvise-core/src/model/mod.rs @@ -2,6 +2,7 @@ pub mod category; pub mod cell; pub mod id; pub mod record; +pub mod source; pub mod symbol; pub mod types; diff --git a/crates/improvise-core/src/model/source.rs b/crates/improvise-core/src/model/source.rs new file mode 100644 index 00000000..22d3cb5f --- /dev/null +++ b/crates/improvise-core/src/model/source.rs @@ -0,0 +1,558 @@ +//! Where cell values are read from: a read-only trait, and the aggregate it answers in. +//! Reasoning: `context/code/source.md` (ADR 003, ADR 009 clause 2; improvise-qddo). + +use super::cell::{CellKey, CellValue, DataStore}; +use crate::formula::AggFunc; +use numtower::ExactSum; + +/// The stored cells one read asks about: those that contain every pair of `base`, and, for each +/// span, one of its listed items. `exact` admits only a cell with no other coordinate. +#[derive(Debug, Clone, Copy)] +pub struct Select<'a> { + pub base: &'a [(String, String)], + pub spans: &'a [(&'a str, Vec<&'a str>)], + pub exact: bool, +} + +impl<'a> Select<'a> { + /// Every cell containing `base`. + pub fn matching(base: &'a [(String, String)]) -> Self { + Select { + base, + spans: &[], + exact: false, + } + } + + /// Every cell containing `base` and one listed item of each span. + pub fn spanning( + base: &'a [(String, String)], + spans: &'a [(&'a str, Vec<&'a str>)], + exact: bool, + ) -> Self { + Select { base, spans, exact } + } +} + +/// What a source reports of the numbers in a read: count, exact sum, extremes. It merges. +/// Why it is enough: `context/code/source.md`. +#[derive(Debug, Clone, Default)] +pub struct Numbers { + count: u64, + sum: ExactSum, + least: Option, + greatest: Option, +} + +impl Numbers { + pub fn of(values: impl IntoIterator) -> Self { + let mut numbers = Numbers::default(); + values.into_iter().for_each(|x| numbers.add(x)); + numbers + } + + pub fn add(&mut self, x: f64) { + self.count += 1; + self.sum.add(x, 1); + if !x.is_nan() { + self.least = Some(self.least.map_or(x, |l| order_min(l, x))); + self.greatest = Some(self.greatest.map_or(x, |g| order_max(g, x))); + } + } + + pub fn merge(&mut self, other: &Numbers) { + self.count += other.count; + self.sum.absorb(&other.sum); + self.least = merged(self.least, other.least, order_min); + self.greatest = merged(self.greatest, other.greatest, order_max); + } + + pub fn count(&self) -> u64 { + self.count + } + + /// The sum correctly rounded once, signed as IEEE signs a zero total; `+0.0` over none. + pub fn total(&self) -> f64 { + self.sum.value() + } + + /// `None` over no numbers: an empty read is absent, not zero. `Min` and `Max` skip NaN, and + /// are NaN when nothing else is left; zeros order by sign. + pub fn reduce(&self, func: &AggFunc) -> Option { + (self.count > 0).then(|| match func { + AggFunc::Sum => self.sum.value(), + AggFunc::Avg => self.sum.mean(self.count), + AggFunc::Min => self.least.unwrap_or(f64::NAN), + AggFunc::Max => self.greatest.unwrap_or(f64::NAN), + AggFunc::Count => self.count as f64, + }) + } +} + +fn order_min(a: f64, b: f64) -> f64 { + if b.total_cmp(&a).is_lt() { b } else { a } +} + +fn order_max(a: f64, b: f64) -> f64 { + if b.total_cmp(&a).is_gt() { b } else { a } +} + +fn merged(a: Option, b: Option, pick: fn(f64, f64) -> f64) -> Option { + match (a, b) { + (Some(x), Some(y)) => Some(pick(x, y)), + (x, y) => x.or(y), + } +} + +/// The values that are not numbers, as far as unanimity needs them. +#[derive(Debug, Clone, Default, PartialEq)] +enum Others { + #[default] + None, + One(CellValue), + Mixed, +} + +/// A read of cells as a source reports it: the numbers, and whether the rest agree on one +/// value. It reduces as raw values do (core.md §2); none at all is absent. +#[derive(Debug, Clone, Default)] +pub struct Summary { + numbers: Numbers, + others: Others, +} + +impl Summary { + pub fn of<'a>(values: impl IntoIterator) -> Self { + let mut summary = Summary::default(); + values.into_iter().for_each(|v| summary.add(v)); + summary + } + + pub fn add(&mut self, value: &CellValue) { + match value.as_f64() { + Some(x) => self.numbers.add(x), + None => self.add_other(value), + } + } + + 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, + }; + } + + 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, + } + } + + /// Only the numbers: what a formula-level aggregate pools. + pub fn numbers(&self) -> &Numbers { + &self.numbers + } + + 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()), + _ => Some(CellValue::Error("mixed".to_string())), + } + } +} + +/// The read side of cell storage (ADR 003). Nothing here writes, so a source that cannot be +/// written to is not one that fails at runtime: it has no method to fail in. +pub trait CellSource { + /// The value stored at exactly `key`. + fn value_at(&self, key: &CellKey) -> Option; + + /// The summary of the values `select` names. + fn summarize(&self, select: &Select) -> Summary; + + /// The keys of the cells containing every pair of `partial`. + fn keys<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a>; + + /// The cells containing every pair of `partial`, with their values. + fn cells<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a>; +} + +impl CellSource for DataStore { + fn value_at(&self, key: &CellKey) -> Option { + self.get(key).cloned() + } + + fn summarize(&self, select: &Select) -> Summary { + let values = match (select.spans.is_empty(), select.exact) { + (true, true) => Vec::from_iter(self.get(&CellKey::new(select.base.to_vec()))), + (true, false) => self.matching_values(select.base), + (false, _) => self.spanned_values(select.base, select.spans, select.exact), + }; + Summary::of(values) + } + + fn keys<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + Box::new(self.cells(partial).map(|(key, _)| key)) + } + + fn cells<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + match partial.is_empty() { + true => Box::new(self.iter_cells().map(|(k, v)| (k, v.clone()))), + false => Box::new( + self.matching_cells(partial) + .into_iter() + .map(|(k, v)| (k, v.clone())), + ), + } + } +} + +/// A table of rows read by scanning, the way a query engine with no index would. A second +/// implementation of [`CellSource`] that shares no code with [`DataStore`] but the summary. +#[cfg(any(test, feature = "test-support"))] +#[derive(Debug, Clone, Default)] +pub struct TableSource { + pub rows: Vec<(CellKey, CellValue)>, +} + +#[cfg(any(test, feature = "test-support"))] +impl TableSource { + fn holds(key: &CellKey, select: &Select) -> bool { + let has = |(cat, item): &(String, String)| key.get(cat) == Some(item.as_str()); + let spanned = |(cat, items): &(&str, Vec<&str>)| { + key.get(cat).is_some_and(|held| items.contains(&held)) + }; + select.base.iter().all(has) + && select.spans.iter().all(spanned) + && (!select.exact || key.0.len() == select.base.len() + select.spans.len()) + } +} + +#[cfg(any(test, feature = "test-support"))] +impl CellSource for TableSource { + fn value_at(&self, key: &CellKey) -> Option { + self.rows + .iter() + .find(|(k, _)| k == key) + .map(|(_, v)| v.clone()) + } + + fn summarize(&self, select: &Select) -> Summary { + let held = self.rows.iter().filter(|(key, _)| Self::holds(key, select)); + Summary::of(held.map(|(_, v)| v)) + } + + fn keys<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + Box::new(self.cells(partial).map(|(key, _)| key)) + } + + fn cells<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + let select = Select::matching(partial); + let held = self.rows.iter(); + Box::new( + held.filter(move |(key, _)| Self::holds(key, &select)) + .cloned(), + ) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const FUNCS: [AggFunc; 5] = [ + AggFunc::Sum, + AggFunc::Avg, + AggFunc::Min, + AggFunc::Max, + AggFunc::Count, + ]; + + fn pool() -> Vec { + let text = |s: &str| CellValue::Text(s.into()); + vec![ + CellValue::Number(1.5), + CellValue::Number(-0.0), + CellValue::Number(0.0), + CellValue::Number(f64::NAN), + CellValue::Number(f64::INFINITY), + text("a"), + text("b"), + CellValue::Error("e".into()), + ] + } + + /// Every sequence of the pool up to `len` values. + fn sequences(len: usize) -> Vec> { + let mut all: Vec> = vec![vec![]]; + let mut last = all.clone(); + for _ in 0..len { + let extend = |s: &Vec| { + pool() + .into_iter() + .map(|v| [s.clone(), vec![v]].concat()) + .collect::>() + }; + last = last.iter().flat_map(extend).collect(); + all.extend(last.clone()); + } + all + } + + /// The reduction as it was before the source moved it, kept to compare against. + fn reference(values: &[&CellValue], func: &AggFunc) -> Option { + let numbers: Vec = values.iter().filter_map(|v| v.as_f64()).collect(); + values.split_first().and_then(|(first, rest)| { + if numbers.len() == values.len() { + let mut sum = ExactSum::new(); + numbers.iter().for_each(|v| sum.add(*v, 1)); + let extreme = |order: fn(&f64, &f64) -> std::cmp::Ordering| { + let finite = numbers.iter().copied().filter(|v| !v.is_nan()); + finite.min_by(order).unwrap_or(f64::NAN) + }; + Some(CellValue::Number(match func { + AggFunc::Sum => sum.value(), + AggFunc::Avg => sum.mean(numbers.len() as u64), + AggFunc::Min => extreme(f64::total_cmp), + AggFunc::Max => extreme(|a, b| b.total_cmp(a)), + AggFunc::Count => numbers.len() as f64, + })) + } else if rest.iter().all(|v| v == first) { + Some((*first).clone()) + } else { + Some(CellValue::Error("mixed".to_string())) + } + }) + } + + fn same(a: &Option, b: &Option) -> bool { + match (a, b) { + (Some(CellValue::Number(x)), Some(CellValue::Number(y))) => { + x.to_bits() == y.to_bits() || (x.is_nan() && y.is_nan()) + } + _ => a == b, + } + } + + #[test] + fn a_summary_reduces_as_the_values_did() { + for values in sequences(3) { + let refs: Vec<&CellValue> = values.iter().collect(); + for func in &FUNCS { + let want = reference(&refs, func); + let got = Summary::of(&values).reduce(func); + assert!( + same(&want, &got), + "{func:?} of {values:?}: {want:?} != {got:?}" + ); + } + } + } + + #[test] + fn a_merge_of_summaries_is_the_summary_of_the_union() { + let all = sequences(2); + for (a, b) in all.iter().flat_map(|a| all.iter().map(move |b| (a, b))) { + let mut merged = Summary::of(a); + merged.merge(&Summary::of(b)); + let union: Vec<&CellValue> = a.iter().chain(b).collect(); + for func in &FUNCS { + let (want, got) = (reference(&union, func), merged.reduce(func)); + assert!( + same(&want, &got), + "{func:?} of {a:?} + {b:?}: {want:?} != {got:?}" + ); + } + } + } + + #[test] + fn numbers_pool_what_the_formula_aggregate_pools() { + let xs = [1.0, -0.0, f64::NAN, 3.0, 0.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!(pooled.total().to_bits(), whole.total().to_bits()); + } + assert_eq!(Numbers::default().total().to_bits(), 0.0f64.to_bits()); + assert_eq!(Numbers::default().reduce(&AggFunc::Sum), None); + } + + fn pair(c: &str, i: &str) -> (String, String) { + (c.to_string(), i.to_string()) + } + + /// Rows of several key shapes over three categories, with text among the numbers. + fn rows() -> Vec<(CellKey, CellValue)> { + let mut rows = Vec::new(); + let records = [("E", "A"), ("E", "B"), ("W", "A"), ("W", "B")]; + for (n, (r, p)) in records.iter().enumerate() { + for (m, v) in [("Rev", n as f64 + 1.0), ("Cost", 0.5 * n as f64)] { + let key = CellKey::new(vec![pair("R", r), pair("P", p), pair("_Measure", m)]); + rows.push((key, CellValue::Number(v))); + } + } + let note = CellKey::new(vec![pair("R", "E"), pair("_Measure", "Note")]); + rows.push((note, CellValue::Text("x".into()))); + let lone = CellKey::new(vec![pair("R", "W"), pair("_Measure", "Rev")]); + rows.push((lone, CellValue::Number(100.0))); + rows + } + + fn store_of(rows: &[(CellKey, CellValue)]) -> DataStore { + let mut store = DataStore::new(); + rows.iter() + .for_each(|(k, v)| store.set(k.clone(), v.clone())); + store + } + + type Spans = Vec<(String, Vec)>; + + /// Every subset of every stored key's pairs as a base, each with no span, with one + /// category spanned over each of several item lists, and with two spanned. + fn selects(rows: &[(CellKey, CellValue)]) -> Vec<(Vec<(String, String)>, Spans)> { + let lists: Vec> = [ + vec!["E"], + vec!["E", "W"], + vec!["A", "B"], + vec!["Rev", "Note"], + vec!["Cost", "Rev", "Note"], + vec!["none"], + ] + .iter() + .map(|l| l.iter().map(|s| s.to_string()).collect()) + .collect(); + let mut out = Vec::new(); + for (key, _) in rows { + for mask in 0..(1u32 << key.0.len()) { + let keep = |(i, _): &(usize, &(String, String))| mask >> i & 1 == 1; + let base: Vec<_> = key.0.iter().enumerate().filter(keep).collect(); + let base: Vec<_> = base.into_iter().map(|(_, p)| p.clone()).collect(); + out.push((base.clone(), vec![])); + let all = ["R", "P", "_Measure"].map(String::from); + let free: Vec<_> = all + .iter() + .filter(|c| base.iter().all(|(b, _)| b != *c)) + .collect(); + for cat in free.iter().copied() { + for list in &lists { + out.push((base.clone(), vec![(cat.clone(), list.clone())])); + for other in free.iter().copied().filter(|o| *o != cat) { + let both = vec![ + (cat.clone(), list.clone()), + (other.clone(), lists[1].clone()), + ]; + out.push((base.clone(), both)); + } + } + } + } + } + out + } + + #[test] + fn the_store_and_a_scanned_table_answer_every_select_alike() { + let rows = rows(); + let (store, table) = (store_of(&rows), TableSource { rows: rows.clone() }); + let mut asked = 0; + for (base, spans) in selects(&rows) { + let spans: Vec<(&str, Vec<&str>)> = spans + .iter() + .map(|(c, i)| (c.as_str(), i.iter().map(String::as_str).collect())) + .collect(); + for exact in [false, true] { + let select = Select::spanning(&base, &spans, exact); + for func in &FUNCS { + let a = store.summarize(&select).reduce(func); + let b = table.summarize(&select).reduce(func); + assert!( + same(&a, &b), + "{func:?} {base:?} {spans:?} exact={exact}: {a:?} != {b:?}" + ); + asked += 1; + } + } + } + assert!(asked > 5000, "the enumeration shrank to {asked}"); + } + + #[test] + fn the_store_and_a_scanned_table_list_the_same_cells() { + let rows = rows(); + let (store, table) = (store_of(&rows), TableSource { rows: rows.clone() }); + for (base, _) in selects(&rows) { + let sorted = |source: &dyn CellSource| { + let mut cells: Vec<_> = source.cells(&base).collect(); + cells.sort_by(|a, b| a.0.cmp(&b.0)); + cells + }; + assert_eq!(sorted(&store), sorted(&table), "cells under {base:?}"); + let mut keys: Vec<_> = store.keys(&base).collect(); + keys.sort(); + let listed: Vec<_> = sorted(&table).into_iter().map(|c| c.0).collect(); + assert_eq!(keys, listed, "keys under {base:?}"); + } + for (key, value) in &rows { + assert_eq!(store.value_at(key).as_ref(), Some(value)); + assert_eq!(table.value_at(key).as_ref(), Some(value)); + } + let absent = CellKey::new(vec![pair("R", "nowhere")]); + assert_eq!( + (store.value_at(&absent), table.value_at(&absent)), + (None, None) + ); + } + + #[test] + fn an_exact_select_with_no_span_names_one_cell() { + let store = store_of(&rows()); + let note = vec![pair("R", "E"), pair("_Measure", "Note")]; + let one = store.summarize(&Select::spanning(¬e, &[], true)); + assert_eq!(one.reduce(&AggFunc::Sum), Some(CellValue::Text("x".into()))); + let wide = vec![pair("R", "E")]; + let loose = store.summarize(&Select::matching(&wide)); + let mixed = Some(CellValue::Error("mixed".into())); + assert_eq!( + loose.reduce(&AggFunc::Sum), + mixed, + "E holds text and numbers" + ); + let exact = store.summarize(&Select::spanning(&wide, &[], true)); + assert_eq!( + exact.reduce(&AggFunc::Sum), + None, + "no cell is keyed by R=E alone" + ); + } +} diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 9c87fb1b..256b13ac 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -5,6 +5,7 @@ use indexmap::{IndexMap, IndexSet}; use super::category::{Category, CategoryId, CategoryKind, MEASURE_CATEGORY}; use super::cell::{CellKey, CellValue, DataStore}; +use super::source::{CellSource, Numbers, Select}; use crate::formula::{AggFunc, Expr, Filter, Formula, FormulaId}; /// Regular categories a model may hold; labels and virtuals do not count. @@ -15,30 +16,6 @@ pub const MAX_CATEGORIES: usize = 24; /// They must agree; why, and why 1e-10: context/code/core.md §4. const FLOAT_EQ_EPSILON: f64 = 1e-10; -/// The sum correctly rounded once, whatever the order (ADR 009 clause 8), signed as IEEE -/// signs a zero total. -fn exact_sum(values: &[f64]) -> f64 { - exact(values).value() -} - -/// The mean correctly rounded once: the exact total over the count, not a rounded sum divided. -fn exact_mean(values: &[f64]) -> f64 { - exact(values).mean(values.len() as u64) -} - -fn exact(values: &[f64]) -> numtower::ExactSum { - let mut sum = numtower::ExactSum::new(); - values.iter().for_each(|v| sum.add(*v, 1)); - sum -} - -/// The least of `values` under `order`, skipping NaN; NaN if nothing else is left. Zeros order -/// by sign (`-0.0` below `+0.0`), so the answer does not depend on the order or the platform. -fn extreme(values: &[f64], order: impl Fn(&f64, &f64) -> std::cmp::Ordering) -> f64 { - let numbers = values.iter().copied().filter(|v| !v.is_nan()); - numbers.min_by(order).unwrap_or(f64::NAN) -} - /// Whether two cached cells are the same value for a fixed point: numbers by bits, so a zero /// that changes sign is a change, except that every NaN is one value, so a NaN can settle. pub fn settled(a: Option<&CellValue>, b: Option<&CellValue>) -> bool { @@ -52,36 +29,16 @@ pub fn settled(a: Option<&CellValue>, b: Option<&CellValue>) -> bool { /// Reduce raw values. `None` on empty: an empty cell reads as **absent**, not /// as zero. Both aggregation paths share it (improvise-kdp, core.md §2). +#[cfg(test)] fn apply_agg(values: &[f64], func: &AggFunc) -> Option { - (!values.is_empty()).then(|| match func { - AggFunc::Sum => exact_sum(values), - AggFunc::Avg => exact_mean(values), - AggFunc::Min => extreme(values, f64::total_cmp), - AggFunc::Max => extreme(values, |a, b| b.total_cmp(a)), - AggFunc::Count => values.len() as f64, - }) -} - -/// Reduce a cell's raw values. Numbers through [`apply_agg`]; anything else -/// by **unanimity**, else `Error("mixed")`. Branch order matters — §2. -fn aggregate_cell_values(values: &[&CellValue], func: &AggFunc) -> Option { - let numbers: Vec = values.iter().filter_map(|v| v.as_f64()).collect(); - values.split_first().and_then(|(first, rest)| { - if numbers.len() == values.len() { - apply_agg(&numbers, func).map(CellValue::Number) - } else if rest.iter().all(|v| v == first) { - Some((*first).clone()) - } else { - Some(CellValue::Error("mixed".to_string())) - } - }) + Numbers::of(values.iter().copied()).reduce(func) } /// The member records behind `partial`, in `DataStore` insertion order and /// without repeats — the coordinate tuples minus `_Measure` (§13). fn member_records(model: &Model, partial: &CellKey) -> Vec { let mut seen: IndexSet = IndexSet::new(); - for (key, _) in model.data.matching_cells(&partial.0) { + for key in model.source().keys(&partial.0) { seen.insert(key.without(MEASURE_CATEGORY)); } seen.into_iter().collect() @@ -107,16 +64,16 @@ fn eval_agg( None => c.clone(), }) .collect(); - let values = distinct - .iter() - .map(|c| agg_values(model, c, target_category, inner, agg_filter, eval_at)) - .collect::, String>>()? - .concat(); + let mut pooled = Numbers::default(); + for context in &distinct { + let found = agg_values(model, context, target_category, inner, agg_filter, eval_at)?; + pooled.merge(&found); + } match func { - AggFunc::Sum => Ok(exact_sum(&values)), - AggFunc::Count => Ok(values.len() as f64), + AggFunc::Sum => Ok(pooled.total()), + AggFunc::Count => Ok(pooled.count() as f64), AggFunc::Avg | AggFunc::Min | AggFunc::Max => { - apply_agg(&values, func).ok_or_else(|| "empty".to_string()) + pooled.reduce(func).ok_or_else(|| "empty".to_string()) } } } @@ -130,7 +87,7 @@ fn agg_values( inner: &Expr, agg_filter: Option<&Filter>, eval_at: &dyn Fn(&Expr, &CellKey) -> Result, -) -> Result, String> { +) -> Result { let mut partial = context.without(target_category); // The fast path is the fold specialised to a ref that names a STORED // cell; where it does not, the premise fails — §13, s08t, s2w3. @@ -156,11 +113,10 @@ fn agg_values( // A bare ref asks for the values that exist; anything else asks the // expression of each member record and folds the answers — §13. true => Ok(model - .data - .matching_values(&partial.0) - .into_iter() - .filter_map(|v| v.as_f64()) - .collect()), + .source() + .summarize(&Select::matching(&partial.0)) + .numbers() + .clone()), false => member_records(model, &partial) .into_iter() .map(|record| { @@ -170,7 +126,8 @@ fn agg_values( .fold(partial.clone(), |k, (cat, item)| k.with(cat, item)); eval_at(inner, &at) }) - .collect(), + .collect::, String>>() + .map(Numbers::of), } } @@ -765,10 +722,21 @@ impl Model { } match self.formula_for(key) { Some(formula) => self.eval_formula_depth(formula, key, depth - 1), - None => self.data.get(key).cloned(), + None => self.source().value_at(key), } } + /// Where raw cell values are read from: the owned store. + pub fn source(&self) -> &dyn CellSource { + &self.data + } + + /// `select`'s cells reduced by `key`'s measure aggregation: numbers by the function, anything + /// else by unanimity, else `Error("mixed")` (core.md §2). + fn read(&self, select: &Select, key: &CellKey) -> Option { + self.source().summarize(select).reduce(self.agg_for(key)) + } + /// The aggregation to apply to a cell's raw values, from the measure /// coordinate's entry in `measure_agg`. Defaults to `Sum`. fn agg_for(&self, key: &CellKey) -> &AggFunc { @@ -898,7 +866,7 @@ impl Model { if let Some(cached) = self.formula_cache.get(key) { return Some(cached.clone()); } - aggregate_cell_values(&self.data.matching_values(&key.0), self.agg_for(key)) + self.read(&Select::matching(&key.0), key) } /// Each category `key` folds, with its covered members: a collapsed GROUP it @@ -979,10 +947,8 @@ impl Model { agg: Aggregation, ) -> Option { let exact = agg == Aggregation::Exact; - let values = self - .data - .spanned_values(&expansion.base.0, &expansion.spans, exact); - aggregate_cell_values(&values, self.agg_for(key)) + let select = Select::spanning(&expansion.base.0, &expansion.spans, exact); + self.read(&select, key) } /// A formula at a group key, its refs read through the fold — the value the @@ -1042,7 +1008,7 @@ impl Model { // (improvise-6os replaced an O(n^2) Vec::contains). let mut stems: IndexSet = IndexSet::new(); for (target_cat, _) in &formula_cats { - for (key, _) in self.data.iter_cells() { + for key in self.source().keys(&[]) { let stem = key.without(target_cat); // Strip none_cats too: the grid queries without them. let mut stripped = stem; @@ -1262,9 +1228,9 @@ impl Model { /// Aggregate raw data (never formulas) over hidden dimensions. fn aggregate_raw(&self, key: &CellKey, none_cats: &[String]) -> Option { if none_cats.is_empty() { - return self.data.get(key).cloned(); + return self.source().value_at(key); } - aggregate_cell_values(&self.data.matching_values(&key.0), self.agg_for(key)) + self.read(&Select::matching(&key.0), key) } fn eval_formula_depth( @@ -1280,7 +1246,7 @@ impl Model { .map(|v| v == filter.item.as_str()) .unwrap_or(false); if !matches { - return self.data.get(context).cloned(); + return self.source().value_at(context); } } diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index d7187d22..5c8ae905 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -91,20 +91,7 @@ pub fn drill_constraint(model: &Model, key: &CellKey) -> CellKey { /// The frozen record list a drill into `constraint` shows. **One home**: a /// second derivation would freeze a different order — core.md §8, gotcha 18. pub fn drill_snapshot(model: &Model, constraint: &CellKey) -> Vec { - let cells: Vec<(CellKey, CellValue)> = match constraint.0.is_empty() { - true => model - .data - .iter_cells() - .map(|(k, v)| (k, v.clone())) - .collect(), - false => model - .data - .matching_cells(&constraint.0) - .into_iter() - .map(|(k, v)| (k, v.clone())) - .collect(), - }; - RecordRow::group(cells) + RecordRow::group(model.source().cells(&constraint.0)) } /// The records row `record` is, as the records grid groups it; empty when no @@ -729,13 +716,7 @@ impl GridLayout { } fn group_records(model: &Model, page_coords: &[(String, String)]) -> Vec { - RecordRow::group( - model - .data - .matching_cells(page_coords) - .into_iter() - .map(|(k, v)| (k, v.clone())), - ) + RecordRow::group(model.source().cells(page_coords)) } /// `[coordinates…, +cat, measures…, +]`. Formula targets are not columns: diff --git a/scripts/build-doc-site.mjs b/scripts/build-doc-site.mjs index 325ce8fd..09d58143 100644 --- a/scripts/build-doc-site.mjs +++ b/scripts/build-doc-site.mjs @@ -72,6 +72,7 @@ const SECTIONS = [ ["context/code/zset-ffi.md", "code/zset-ffi", "zset-ffi, the C ABI"], ["context/code/delivery.md", "code/delivery", "delivery, notices and corrections"], ["context/code/improvise-incremental.md", "code/improvise-incremental", "improvise-incremental, the plug"], + ["context/code/source.md", "code/source", "CellSource, where cells are read from"], ["context/code/kb-tool.md", "code/kb-tool", "The kb tool"], ["context/code/tooling.md", "code/tooling", "The repository's own checkers"], ], From 7a3e0c5b5534b212ad5dddae46ecb2edc5f7a635 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 17:58:17 -0700 Subject: [PATCH 2/5] style(core): the test-only reference reduction maps rather than and_thens (improvise-qddo) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-core/src/model/source.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/improvise-core/src/model/source.rs b/crates/improvise-core/src/model/source.rs index 22d3cb5f..f25fea70 100644 --- a/crates/improvise-core/src/model/source.rs +++ b/crates/improvise-core/src/model/source.rs @@ -325,7 +325,7 @@ mod tests { /// The reduction as it was before the source moved it, kept to compare against. fn reference(values: &[&CellValue], func: &AggFunc) -> Option { let numbers: Vec = values.iter().filter_map(|v| v.as_f64()).collect(); - values.split_first().and_then(|(first, rest)| { + values.split_first().map(|(first, rest)| { if numbers.len() == values.len() { let mut sum = ExactSum::new(); numbers.iter().for_each(|v| sum.add(*v, 1)); @@ -333,17 +333,17 @@ mod tests { let finite = numbers.iter().copied().filter(|v| !v.is_nan()); finite.min_by(order).unwrap_or(f64::NAN) }; - Some(CellValue::Number(match func { + CellValue::Number(match func { AggFunc::Sum => sum.value(), AggFunc::Avg => sum.mean(numbers.len() as u64), AggFunc::Min => extreme(f64::total_cmp), AggFunc::Max => extreme(|a, b| b.total_cmp(a)), AggFunc::Count => numbers.len() as f64, - })) + }) } else if rest.iter().all(|v| v == first) { - Some((*first).clone()) + (*first).clone() } else { - Some(CellValue::Error("mixed".to_string())) + CellValue::Error("mixed".to_string()) } }) } From 4a49398f1aa9413abcab0efbc1e65989add550bf Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:52:58 -0700 Subject: [PATCH 3/5] 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()); + } } From 8ba4bd0a6cfee1698cd05bb1403d2a3fbc2c9023 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 19:30:22 -0700 Subject: [PATCH 4/5] 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] From f7404778b003eeb1598570ad3d80f6db2ceb94ed Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 19:48:34 -0700 Subject: [PATCH 5/5] fix(core): the exactness test merges both ways and from_parts refuses sums that contradict their counts (improvise-qddo) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/source.md | 3 ++- context/test-evidence/core.md | 9 ++++--- crates/improvise-core/src/model/source.rs | 30 +++++++++++++++++++---- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/context/code/source.md b/context/code/source.md index 10a91cc5..fb333933 100644 --- a/context/code/source.md +++ b/context/code/source.md @@ -79,5 +79,6 @@ lists, and two), at both widths and for all five functions: over 5,000 triples. `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 +least above greatest, one extreme without the other, a sum with no numbers, no extremes beside a sum +that is not NaN, 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 0791c7cb..1647dbfd 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -2317,7 +2317,8 @@ lost). The first two also fail `aggregating_differing_text_values_is_an_explicit 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. +compared as bits), `summaries_built_from_parts_merge_as_the_union_does` and the fixture test below, by +name: four tests, for each of the three round-then-pool variants. 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 @@ -2369,12 +2370,14 @@ Review of #356 (Opus, Codex): `from_parts` repaired some impossible states and a 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. +others, a sum with no numbers allowed, and no extremes beside a sum that is not NaN allowed. ### 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. +by part is `0.0`; pooled exactly it is `1.0`. The merge runs in both orders, so whichever part a rival +rounds is the inexact one: rounding only the other part, only self, or both each fail it (the first +version merged one way and a rival rounding only the exact part passed it; Opus caught that). ### 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 384e9ca0..c468c621 100644 --- a/crates/improvise-core/src/model/source.rs +++ b/crates/improvise-core/src/model/source.rs @@ -60,7 +60,13 @@ impl Numbers { greatest: Option, ) -> Result { let bad = |why| Err(BadParts(why)); + if count == 0 && sum != ExactSum::new() { + return bad("a sum with no numbers"); + } match (least, greatest) { + (None, None) if count > 0 && !sum.value().is_nan() => { + return bad("no extremes but a sum that is not NaN"); + } (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"), @@ -462,13 +468,15 @@ mod tests { #[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" + "the fixture is inexact: rounded parts lose the 1" ); - assert_eq!(pooled.total(), 1.0); + for (first, second) in [([1e16, 1.0], [-1e16, 0.0]), ([-1e16, 0.0], [1e16, 1.0])] { + let mut pooled = Numbers::of(first); + pooled.merge(&Numbers::of(second)); + assert_eq!(pooled.total(), 1.0, "{first:?} then {second:?}"); + } } fn pair(c: &str, i: &str) -> (String, String) { @@ -701,7 +709,19 @@ mod tests { 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(); + let mut one = ExactSum::new(); + one.add(1.0, 1); + assert_eq!( + refused(Numbers::from_parts(0, one.clone(), None, None)), + "a sum with no numbers" + ); + assert_eq!( + refused(Numbers::from_parts(2, one, None, None)), + "no extremes but a sum that is not NaN" + ); + let mut nan = ExactSum::new(); + nan.add(f64::NAN, 1); + let ok = Numbers::from_parts(2, nan, None, None).unwrap(); assert!( ok.reduce(&AggFunc::Min).unwrap().is_nan(), "all NaN: no extremes, a count"