From 5fdf50287112d9240c21125144a932dcb35fd372 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 17:55:14 -0700 Subject: [PATCH 01/14] 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 02/14] 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 315e271905ea55e2979bae25a09636545c6e3517 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:03:20 -0700 Subject: [PATCH 03/14] feat(core): a model can be over a source, so one evaluator serves any of them (improvise-rnak) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/source.md | 27 +- context/test-evidence/core.md | 36 +++ crates/improvise-core/src/model/mod.rs | 3 + crates/improvise-core/src/model/over_tests.rs | 246 ++++++++++++++++++ crates/improvise-core/src/model/source.rs | 4 + crates/improvise-core/src/model/types.rs | 25 +- 6 files changed, 333 insertions(+), 8 deletions(-) create mode 100644 crates/improvise-core/src/model/over_tests.rs diff --git a/context/code/source.md b/context/code/source.md index cffcbe47..80a3b3c5 100644 --- a/context/code/source.md +++ b/context/code/source.md @@ -56,12 +56,29 @@ sum is exact. SQLite's `SUM` is a float sum, so an engine that cannot return an 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()` and `Model::over(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. +`Model::source()` returns the `&dyn CellSource` that the evaluator, the stems of `recompute_formulas`, +the member records of an aggregate over an expression and the records of `GridLayout` read through. +`Model::over(source)` points it at a `SharedSource` (an `Arc`) instead of +the owned `DataStore`. That is the whole evaluator hook (improvise-rnak, bw4p.14's prerequisite): +there is still one evaluator, `Model`'s, and it computes every formula value, over a source whose +cells it never owns. + +1. **A model over a source is read-only.** `set_cell` and `clear_cell` `debug_assert` that no source + is set, because they write the owned store, which is then not read. A release build would drop the + write silently; the assert is the guard, not the design. The design is that a caller who holds a + source writes to the source (ADR 003: the trait has no writes), and the model is built over it. +2. **A shared source changes behind its readers by interior mutability of its own.** The model holds + an `Arc`, so a database connection or a test double behind a lock is the same object the + backend feeds from. +3. **What still reads the owned store**: `get_cell`, persistence, the import wizard and the effects + in `improvise-command`. They are the facade of a document that owns its cells; a model over a source + has none of those to offer, and routing them would change `get_cell`'s `&CellValue` return for 129 + call sites. `Eager::occupancy`, `Shapes::of` and `Stems::build` in `improvise-incremental` are + routed by the PRs that need them. +4. **`Debug` shows only whether a source is set.** A source is not necessarily printable, and two + models over different sources are not told apart by it. ## `TableSource` diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index c6fac22a..69226c41 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -2344,3 +2344,39 @@ 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. + +### model::over_tests::a_model_over_a_source_evaluates_as_the_model_that_owns_the_cells + +improvise-rnak. A model with its store emptied and `over` a `TableSource` of the same rows is asked +every read the evaluator makes (exact and over-hidden values, covered by a view that hides East, +spanned over none, one and two categories) at every sub-key of every stored key, a group named in +place of a member, and every formula target; the answers must agree to the bit. The fixture has a +ref, an aggregate of a ref, an aggregate of an expression, a filtered formula, a cycle and a +formula over formulas, and 2,000+ answers are compared. Each rival replaces one read with the owned +store, and fails it by name: the member records of an aggregate over an expression, the pooled +formula aggregate's raw read, the fold of a group, the exact read, and the stored cell a filtered +formula falls back to (a `Bonus` cell stored where its `WHERE` does not hold; before that cell +was added to the fixture this rival passed). + +### model::over_tests::the_formula_cells_of_a_model_over_a_source_are_the_owned_models + +`recompute_formulas` over a source under three None sets: the formula cache must equal the owned +model's at every key, and at least 40 cells are present across the three (a cache that is empty on +both sides would pass the comparison). Rival: the stems read from the owned store, which finds no +stem and fills no cell; it fails here and nowhere else. + +### model::over_tests::a_model_over_a_source_does_not_read_its_own_store + +A cell is set in the over-model's own store (the only cell it holds) with a value no fold could +mistake. The exact read and the fold of its measure must be the source's. Rivals: a fold or an exact +read of the owned store fail it. + +### model::over_tests::the_records_a_drill_freezes_are_read_from_the_source + +`drill_snapshot` (records mode's rows) over the empty store and a source. Rival: the unconstrained +branch reads the owned store; it fails here. + +### model::over_tests::a_model_over_a_source_refuses_a_write + +`set_cell` on a model over a source panics in a debug build, naming the reason. Without the assert the +write goes to the owned store and nothing reads it. diff --git a/crates/improvise-core/src/model/mod.rs b/crates/improvise-core/src/model/mod.rs index 29895e30..8d4933bf 100644 --- a/crates/improvise-core/src/model/mod.rs +++ b/crates/improvise-core/src/model/mod.rs @@ -6,4 +6,7 @@ pub mod source; pub mod symbol; pub mod types; +#[cfg(test)] +mod over_tests; + pub use types::{Aggregation, MAX_CATEGORIES, Model}; diff --git a/crates/improvise-core/src/model/over_tests.rs b/crates/improvise-core/src/model/over_tests.rs new file mode 100644 index 00000000..d8078a5d --- /dev/null +++ b/crates/improvise-core/src/model/over_tests.rs @@ -0,0 +1,246 @@ +//! A model over a source reads the source and nothing else; `context/code/source.md`. + +use super::Aggregation; +use super::Model; +use super::cell::{CellKey, CellValue, DataStore}; +use super::source::TableSource; +use crate::formula::parse_formula; +use std::sync::Arc; + +fn key(region: &str, product: &str, measure: &str) -> CellKey { + CellKey::new(vec![ + ("Region".into(), region.into()), + ("Product".into(), product.into()), + ("_Measure".into(), measure.into()), + ]) +} + +const FORMULAS: [&str; 8] = [ + "Profit = Revenue - Cost", + "Weighted = SUM(Revenue * Price)", + "Share = Revenue / SUM(Revenue)", + "Bonus = Revenue * 0.1 WHERE Region = East", + "Peak = MAX(Revenue WHERE Product = A)", + "Loop = Loop + 1", + "Both = Profit + Weighted", + "Mean = AVG(Revenue) + Count", +]; + +/// A group and a loner, text among numbers, and every kind of formula that reads cells. +fn fixture() -> Model { + let mut m = Model::new("over"); + for cat in ["Region", "Product"] { + m.add_category(cat).unwrap(); + } + let region = m.category_mut("Region").unwrap(); + region.add_group(super::category::Group::new("Coastal")); + region.add_item_in_group("East", "Coastal"); + region.add_item_in_group("West", "Coastal"); + region.add_item("Inland"); + for p in ["A", "B"] { + m.category_mut("Product").unwrap().add_item(p); + } + for measure in ["Revenue", "Price", "Cost", "Count", "Note"] { + m.category_mut("_Measure").unwrap().add_item(measure); + } + let regions = ["East", "West", "Inland"]; + for (r, region) in regions.iter().enumerate() { + for (p, product) in ["A", "B"].iter().enumerate() { + let n = (r * 2 + p) as f64; + m.set_cell(key(region, product, "Revenue"), CellValue::Number(10.0 + n)); + m.set_cell(key(region, product, "Price"), CellValue::Number(1.5 + n)); + m.set_cell(key(region, product, "Cost"), CellValue::Number(4.0 - n)); + m.set_cell(key(region, product, "Count"), CellValue::Number(n)); + } + } + m.set_cell(key("East", "A", "Note"), CellValue::Text("n".into())); + m.set_cell(key("West", "A", "Note"), CellValue::Text("n".into())); + m.set_cell(key("East", "B", "Note"), CellValue::Text("x".into())); + m.set_cell(key("Inland", "A", "Revenue"), CellValue::Number(-0.0)); + // Where Bonus's filter does not hold, the stored cell is read in its place. + m.set_cell(key("West", "A", "Bonus"), CellValue::Number(3.0)); + for text in FORMULAS { + m.add_formula(parse_formula(text, "_Measure").unwrap()); + } + m +} + +/// `model`'s cells as the rows of a table, and `model` with no store of its own, over them. +fn lent(model: &Model) -> Model { + let rows = model + .data + .iter_cells() + .map(|(k, v)| (k, v.clone())) + .collect(); + let mut bare = model.clone(); + bare.data = DataStore::new(); + bare.over(Arc::new(TableSource { rows })) +} + +/// Every key worth asking: each stored key with any of its coordinates dropped, a group named +/// in place of its member, and each formula's target at each of those. +fn keys(model: &Model) -> Vec { + let mut out: Vec = Vec::new(); + for (stored, _) in model.data.iter_cells() { + let grouped = stored.clone().with("Region", "Coastal"); + for base in [stored, grouped] { + for mask in 0..(1u32 << base.0.len()) { + let kept = base.0.iter().enumerate(); + let kept = kept.filter(|(i, _)| mask >> i & 1 == 1); + out.push(CellKey::new(kept.map(|(_, p)| p.clone()).collect())); + } + } + } + let targets: Vec = model + .formulas() + .iter() + .flat_map(|f| out.iter().map(|k| k.clone().with("_Measure", &f.target))) + .collect(); + out.extend(targets); + out.sort(); + out.dedup(); + out +} + +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, + } +} + +/// Every read the evaluator makes, asked of both models; how many answers were not absent. +fn compare_reads(owned: &Model, over: &Model) -> (usize, usize) { + let hide_east = |cat: &str, item: &str| !(cat == "Region" && item == "East"); + let spans: [&[String]; 3] = [ + &[], + &["Region".to_string()], + &["Region".to_string(), "Product".to_string()], + ]; + let (mut asked, mut present) = (0, 0); + for key in keys(owned) { + let mut pairs = Vec::new(); + for agg in [Aggregation::Exact, Aggregation::OverHidden] { + pairs.push(( + owned.evaluate_aggregated(&key, agg), + over.evaluate_aggregated(&key, agg), + )); + pairs.push(( + owned.evaluate_covering(&key, agg, &hide_east), + over.evaluate_covering(&key, agg, &hide_east), + )); + for span in spans { + pairs.push(( + owned.evaluate_spanning(&key, span, agg, &hide_east), + over.evaluate_spanning(&key, span, agg, &hide_east), + )); + } + } + for (a, b) in pairs { + assert!(same(&a, &b), "{key}: owned {a:?} != over a source {b:?}"); + asked += 1; + present += usize::from(a.is_some()); + } + } + (asked, present) +} + +#[test] +fn a_model_over_a_source_evaluates_as_the_model_that_owns_the_cells() { + let owned = fixture(); + let over = lent(&owned); + let (asked, present) = compare_reads(&owned, &over); + assert!(asked > 2000, "the enumeration shrank to {asked}"); + assert!( + present > asked / 2, + "{present} of {asked} answers were not absent" + ); +} + +#[test] +fn the_formula_cells_of_a_model_over_a_source_are_the_owned_models() { + let mut total = 0; + for none in [ + vec![], + vec!["Product".to_string()], + vec!["Region".to_string(), "Product".to_string()], + ] { + let (mut owned, mut over) = (fixture(), lent(&fixture())); + owned.recompute_formulas(&none); + over.recompute_formulas(&none); + let mut cells = 0; + for key in keys(&owned) { + let (a, b) = ( + owned.formula_cell(&key).cloned(), + over.formula_cell(&key).cloned(), + ); + assert!( + same(&a, &b), + "{none:?} {key}: owned {a:?} != over a source {b:?}" + ); + cells += usize::from(a.is_some()); + } + assert!( + cells > 5, + "{none:?}: only {cells} formula cells were present" + ); + total += cells; + compare_reads(&owned, &over); + } + assert!(total > 40, "only {total} formula cells were compared"); +} + +#[test] +fn a_model_over_a_source_does_not_read_its_own_store() { + let owned = fixture(); + let mut over = lent(&owned); + let poisoned = key("East", "A", "Revenue"); + over.data.set(poisoned.clone(), CellValue::Number(1e9)); + let read = over.evaluate_aggregated(&poisoned, Aggregation::Exact); + assert_eq!( + read, + owned.evaluate_aggregated(&poisoned, Aggregation::Exact) + ); + let revenue = CellKey::new(vec![("_Measure".into(), "Revenue".into())]); + let folded = over.evaluate_aggregated(&revenue, Aggregation::OverHidden); + assert_eq!( + folded, + owned.evaluate_aggregated(&revenue, Aggregation::OverHidden) + ); + assert_ne!(folded, Some(CellValue::Number(1e9)), "the store was folded"); +} + +#[test] +#[should_panic(expected = "read-only")] +fn a_model_over_a_source_refuses_a_write() { + let mut over = lent(&fixture()); + over.set_cell(key("East", "A", "Revenue"), CellValue::Number(1.0)); +} + +#[test] +fn the_records_a_drill_freezes_are_read_from_the_source() { + use crate::view::layout::drill_snapshot; + let (owned, over) = (fixture(), lent(&fixture())); + let constraints = [ + CellKey::new(vec![]), + CellKey::new(vec![("Region".into(), "East".into())]), + CellKey::new(vec![("_Measure".into(), "Revenue".into())]), + CellKey::new(vec![("Region".into(), "Nowhere".into())]), + ]; + let mut rows = 0; + for constraint in &constraints { + let (a, b) = ( + drill_snapshot(&owned, constraint), + drill_snapshot(&over, constraint), + ); + assert_eq!( + format!("{a:?}"), + format!("{b:?}"), + "drill into {constraint}" + ); + rows += a.len(); + } + assert!(rows > 10, "only {rows} records were compared"); +} diff --git a/crates/improvise-core/src/model/source.rs b/crates/improvise-core/src/model/source.rs index f25fea70..d612761d 100644 --- a/crates/improvise-core/src/model/source.rs +++ b/crates/improvise-core/src/model/source.rs @@ -166,6 +166,10 @@ impl Summary { } } +/// A source a model can be over and a backend can hold at once. A source that changes behind +/// its readers (a database) does so through interior mutability of its own. +pub type SharedSource = std::sync::Arc; + /// 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 { diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 256b13ac..d9c161e8 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -5,9 +5,11 @@ 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 super::source::{CellSource, Numbers, Select, SharedSource}; use crate::formula::{AggFunc, Expr, Filter, Formula, FormulaId}; +const READ_ONLY: &str = "a model over a source is read-only: the owned store is not read"; + /// Regular categories a model may hold; labels and virtuals do not count. /// Why 24 and what it costs: context/code/core.md §9 (improvise-felo.1). pub const MAX_CATEGORIES: usize = 24; @@ -362,6 +364,8 @@ pub struct Model { pub measure_agg: HashMap, /// Cached formula evaluation results. Recomputed by `recompute_formulas`. formula_cache: HashMap, + /// Where raw cells are read from instead of `data`, for a model over a live source. + external: Option, } /// Every field, with the two `HashMap`s in key order: a recompute refills the @@ -377,6 +381,7 @@ impl std::fmt::Debug for Model { next_category_id, measure_agg, formula_cache, + external, } = self; let aggs: std::collections::BTreeMap<_, _> = measure_agg.iter().collect(); let mut cache: Vec<_> = formula_cache.iter().collect(); @@ -389,6 +394,7 @@ impl std::fmt::Debug for Model { .field("next_category_id", next_category_id) .field("measure_agg", &aggs) .field("formula_cache", &cache) + .field("external", &external.is_some()) .finish() } } @@ -420,9 +426,17 @@ impl Model { next_category_id: 3, measure_agg: HashMap::new(), formula_cache: HashMap::new(), + external: None, } } + /// This model, reading its raw cells from `source` and not `data`: read-only. + /// Why: `context/code/source.md`. + pub fn over(mut self, source: SharedSource) -> Self { + self.external = Some(source); + self + } + /// Add a pivot category, enforcing `MAX_CATEGORIES` over regular ones. /// The caller registers it on every view — normally `Workbook`. pub fn add_category(&mut self, name: impl Into) -> Result { @@ -538,10 +552,12 @@ impl Model { } pub fn set_cell(&mut self, key: CellKey, value: CellValue) { + debug_assert!(self.external.is_none(), "{READ_ONLY}"); self.data.set(key, value); } pub fn clear_cell(&mut self, key: &CellKey) { + debug_assert!(self.external.is_none(), "{READ_ONLY}"); self.data.remove(key); } @@ -726,9 +742,12 @@ impl Model { } } - /// Where raw cell values are read from: the owned store. + /// Where raw cell values are read from: the source the model is over, else the owned store. pub fn source(&self) -> &dyn CellSource { - &self.data + match &self.external { + Some(source) => &**source, + None => &self.data, + } } /// `select`'s cells reduced by `key`'s measure aggregation: numbers by the function, anything From 9d675c9c33fdf1629d0c6285c4bf54ebf9a34c3a Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:12:04 -0700 Subject: [PATCH 04/14] feat(incremental): a backend over a source that reports its changes (improvise-rnak) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/improvise-incremental.md | 39 ++++ context/code/source.md | 9 + .../test-evidence/improvise-incremental.md | 41 ++++ crates/improvise-core/src/model/source.rs | 12 +- crates/improvise-incremental/src/eager.rs | 4 +- crates/improvise-incremental/src/formulas.rs | 2 +- .../improvise-incremental/src/incremental.rs | 69 +++++- crates/improvise-incremental/src/shapes.rs | 19 +- .../tests/live_source.rs | 213 ++++++++++++++++++ .../tests/random_live.proptest-regressions | 7 + .../tests/random_live.rs | 22 ++ .../tests/support/live.rs | 156 +++++++++++++ .../tests/support/mod.rs | 2 + 13 files changed, 571 insertions(+), 24 deletions(-) create mode 100644 crates/improvise-incremental/tests/live_source.rs create mode 100644 crates/improvise-incremental/tests/random_live.proptest-regressions create mode 100644 crates/improvise-incremental/tests/random_live.rs create mode 100644 crates/improvise-incremental/tests/support/live.rs diff --git a/context/code/improvise-incremental.md b/context/code/improvise-incremental.md index f3604067..0791e1de 100644 --- a/context/code/improvise-incremental.md +++ b/context/code/improvise-incremental.md @@ -388,3 +388,42 @@ that say otherwise (step 4 items 4 and 10, step 5 item 5). Each is a user-visibl - **Overflow of the exact sum to infinity** follows `ExactSum::value` (round to nearest, so an overflowing total is infinite): correct IEEE behaviour. + +## Step 7: live sources (improvise-rnak) + +ADR 009 clause 2 names two kinds of live source: one that reports its changes, which feeds cell deltas +like `DataStore`, and one that answers aggregate queries but reports none, which implements the derived +value by pushdown. The core half of both is in `context/code/source.md` (`CellSource`, `Model::over`). +The brief ordered this half after the evaluator hook; the hook came first because a source that reports +changes still has its formulas evaluated over it. User ruling 2026-10-10: formulas are computed in RAM +from the source's folds, never pushed down; `Derived` stays in this crate. + +### Change-reporting (`Incremental::over`) + +1. **`ChangeSource` is `CellSource` plus `drain_changes(&self)`, and a source's cells are those as of the + last drain.** A shape is built lazily, by a pass over the source, at its first read. If a source + showed a write before reporting it, a shape built in between would hold the write and the next + drain would add it again. So the contract is that nothing is visible until it is drained, which is + what a database does inside a read transaction and what `DataStore` does already (its log is + drained in the step that made the edit). `&self` because a live source is shared with the model that + reads it and changes behind it by interior mutability of its own. +2. **`Incremental::over(model, feed, none_cats)`**: the model is put over the feed (`Model::over`) and + keeps no cells of its own. Its categories, formulas and aggregations are the definition; the + source holds the data. Shapes and stems are built from the feed as it stands. +3. **`sync()` drains and maintains; `apply_changes(&[CellChange])` maintains changes the source already + reflects**, for a source whose changes reach the backend by another route (a trigger table, a + channel). `Maintained::apply` on a fed backend takes definition edits (a formula, an aggregation) + and drains: a cell edit is the source's to make, and `Model::set_cell` refuses it in a debug build. +4. **Freshness is the source's.** ADR 003 left "polled, triggered, or stale until refreshed" open: `sync` + is the poll, and when it is called is the host's decision. Nothing here schedules it. +5. **What a change costs** is what it costs for the owned store: two group updates and two exact-sum + updates per maintained shape, no pass over the source (`the_cost_of_a_reported_change_...` counts the + rows the source serves for snapshots: zero after the build). Formulas are evaluated at the stems the + change reaches, as in step 5. +6. **Three routes into the store were routed through the source**: `Shape::of` (the build) reads + `cells(&[])`, `Stems::build` reads `keys(&[])`, and `Incremental::read` reads `value_at`. `Eager::occupancy` + reads `keys(&[])` too, so an `Eager` over a source would answer it. + +`tests/support/live.rs` holds `LiveTable`, the scripted source (an indexed store behind a lock, with +a pending list and a counter of snapshot rows) and `Fed`, an `Incremental` over one that a differential +can drive: a cell edit becomes a write to the table followed by `sync`. diff --git a/context/code/source.md b/context/code/source.md index 80a3b3c5..7ed4d1d1 100644 --- a/context/code/source.md +++ b/context/code/source.md @@ -87,3 +87,12 @@ 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. + +## `ChangeSource` + +`CellSource` plus `drain_changes(&self) -> Vec`: a source that reports what changed in it. +The contract that matters is that a source's cells are those as of the last drain, so a snapshot read +now plus the changes drained later counts no change twice or not at all. Why, and how `Incremental` +uses it: `context/code/improvise-incremental.md`, step 7. `DataStore` is not a `ChangeSource`: its log +is `&mut` and is drained by the owner in the step that made the edit. A `SharedFeed` is the `Arc` a +backend and a model share. diff --git a/context/test-evidence/improvise-incremental.md b/context/test-evidence/improvise-incremental.md index 55683f27..61011e72 100644 --- a/context/test-evidence/improvise-incremental.md +++ b/context/test-evidence/improvise-incremental.md @@ -801,3 +801,44 @@ User-visible changes this evidence also covers: a cycle that only flips a zero's same cycle over nonzero data always was; a cycle fed a NaN (`A = A + 1`, `A = A` with `A` NaN) settles on NaN instead of `circular`. + +### a_live_source_matches_eager_over_the_corpus_and_its_edits + +improvise-rnak. `Incremental` over a `LiveTable` (the model's cells moved into the table, the model's +own store emptied) against `Eager` over the shipped documents, their views and `edits_for` +(trimmed to three edits above 40 cells, as `corpus_eager` does). Rivals, each failing it by name: `sync` +drains nothing; the model not put over the feed (formulas read an empty store); a shape built from an +empty store; the stems built from the model's own store. + +### changes_named_by_the_caller_are_maintained_as_the_drain_would + +`apply_changes` with the changes the table reports, against `Eager` after each of four edits (an +overwrite, a new record under an item the model lacks, a removal, a text), asking every probe of every +step so far. Rivals: `apply_changes` ignores its changes (only this test and no other fails it); the +stems from the owned store. + +### a_write_is_invisible_until_it_is_drained + +The contract of a `ChangeSource`: a read after a write and before the drain sees the old value, after +`sync` the new one, and a second `sync` changes nothing. Rivals: the table shows a write at once (this +test alone fails it, which is why the contract has a test of its own: the differential cannot see it, +since every shape it asks about was built before the write); `sync` drains nothing. + +### the_cost_of_a_reported_change_does_not_depend_on_the_size_of_the_table + +One overwrite reported by the table, after the shape is built, at 216 and at 864 cells: two group +updates and two sum updates, no build, equal meters, and zero snapshot rows served by the source +for the change. The build itself serves exactly the table (`n * n * 6` rows), so the zero is a count +that could have been positive. Rival: every sync rebuilds the shapes (`builds` and `built_cells` move, +and the table serves the snapshot again). + +### an_overwrite_by_the_same_value_moves_nothing + +A table that reports an overwrite of a cell by its own value leaves the fold where it was (23). Rival: +the model not over the feed fails it with the others. + +### a_live_source_matches_eager_over_random_formula_and_cell_edits + +`Fed` against `Eager` over random formula and cell edits on the formula fixture (no views, so the binary +stays under two seconds). Rivals: `sync` drains nothing, the model not over the feed, a shape built from an +empty store and the stems from the owned store all fail it. diff --git a/crates/improvise-core/src/model/source.rs b/crates/improvise-core/src/model/source.rs index d612761d..5c3fd8a0 100644 --- a/crates/improvise-core/src/model/source.rs +++ b/crates/improvise-core/src/model/source.rs @@ -1,7 +1,7 @@ //! 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 super::cell::{CellChange, CellKey, CellValue, DataStore}; use crate::formula::AggFunc; use numtower::ExactSum; @@ -170,6 +170,16 @@ impl Summary { /// its readers (a database) does so through interior mutability of its own. pub type SharedSource = std::sync::Arc; +/// A source that reports its changes. Its cells are the state after the changes it has drained, +/// so a snapshot read now plus the changes drained later counts no change twice or not at all. +pub trait ChangeSource: CellSource { + /// The changes since the last drain, oldest first; reads see them from now on. + fn drain_changes(&self) -> Vec; +} + +/// A change-reporting source a backend and a model share. +pub type SharedFeed = std::sync::Arc; + /// 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 { diff --git a/crates/improvise-incremental/src/eager.rs b/crates/improvise-incremental/src/eager.rs index 846c950d..7bb674eb 100644 --- a/crates/improvise-incremental/src/eager.rs +++ b/crates/improvise-incremental/src/eager.rs @@ -67,9 +67,9 @@ impl Derived for Eager { held == Some(item.as_str()) || self.group_holds(cat, item, held) }) }; - let cells = self.model.data.iter_cells(); + let cells = self.model.source().keys(&[]); cells - .filter(|(k, _)| named_by(k) && k.0.iter().all(drawn)) + .filter(|k| named_by(k) && k.0.iter().all(drawn)) .count() } } diff --git a/crates/improvise-incremental/src/formulas.rs b/crates/improvise-incremental/src/formulas.rs index f23d4147..2b4b2f73 100644 --- a/crates/improvise-incremental/src/formulas.rs +++ b/crates/improvise-incremental/src/formulas.rs @@ -240,7 +240,7 @@ impl Stems { order: Order::of(model), wide, }; - let cells: Vec = model.data.iter_cells().map(|(k, _)| k).collect(); + let cells: Vec = model.source().keys(&[]).collect(); let presence: Vec<(&CellKey, i64)> = cells.iter().map(|k| (k, 1)).collect(); stems.count(&presence); stems diff --git a/crates/improvise-incremental/src/incremental.rs b/crates/improvise-incremental/src/incremental.rs index 65211405..52b4bdff 100644 --- a/crates/improvise-incremental/src/incremental.rs +++ b/crates/improvise-incremental/src/incremental.rs @@ -9,9 +9,11 @@ use crate::{Derived, Edit, Maintained, Reading, cell_delta}; use improvise_core::formula::ast::AggFunc; use improvise_core::model::category::MEASURE_CATEGORY; use improvise_core::model::cell::{CellChange, CellKey, CellValue}; +use improvise_core::model::source::{ChangeSource, SharedFeed}; use improvise_core::model::types::{Covers, settled}; use improvise_core::model::{Aggregation, Model}; use std::collections::BTreeMap; +use std::sync::Arc; /// How a stem's formulas are evaluated: in dependency order where one pass is exact, or by /// eager's passes always. The two must agree; `Plain` is the reference. @@ -31,6 +33,8 @@ pub struct Incremental { fingerprint: Vec, evaluation: Evaluation, work: Meter, + /// The source the model is over, when its cells are not the model's own. + feed: Option, } static SUM: AggFunc = AggFunc::Sum; @@ -38,6 +42,21 @@ static SUM: AggFunc = AggFunc::Sum; impl Incremental { pub fn new(mut model: Model, none_cats: Vec) -> Self { model.data.record_changes(); + Self::start(model, none_cats, None) + } + + /// A backend over `feed`, which holds the cells and reports their changes; `model`'s own + /// store is not read. Reasoning: `context/code/improvise-incremental.md`, step 7. + pub fn over( + model: Model, + feed: Arc, + none_cats: Vec, + ) -> Self { + let model = model.over(feed.clone()); + Self::start(model, none_cats, Some(feed)) + } + + fn start(model: Model, none_cats: Vec, feed: Option) -> Self { let mut backend = Incremental { model, none_cats, @@ -46,12 +65,37 @@ impl Incremental { fingerprint: Vec::new(), evaluation: Evaluation::default(), work: Meter::default(), + feed, }; backend.rebuild_formulas(); backend.work = Meter::default(); backend } + /// Maintains what the feed has reported since the last drain. + pub fn sync(&mut self) { + let changes = self.drain(); + self.absorb(None, &changes); + } + + /// Maintains `changes`, which the source already reflects. For a source whose changes reach the + /// backend by another route than the drain. + pub fn apply_changes(&mut self, changes: &[CellChange]) { + self.absorb(None, changes); + } + + fn drain(&mut self) -> Vec { + match &self.feed { + Some(feed) => feed.drain_changes(), + None => self.model.data.drain_changes(), + } + } + + fn absorb(&mut self, edit: Option<&Edit>, changes: &[CellChange]) { + self.shapes.apply(&cell_delta(changes), changes); + self.refresh_formulas(edit, changes); + } + pub fn with_evaluation(mut self, evaluation: Evaluation) -> Self { self.evaluation = evaluation; self.rebuild_formulas(); @@ -100,20 +144,24 @@ impl Incremental { let func = Reduce::of(self.func_for(key)); let expansion = Expansion::of(&self.model, key, covers, over); match (expansion.spans.is_empty(), agg) { - (true, Aggregation::Exact) => self.model.data.get(key).cloned(), + (true, Aggregation::Exact) => self.model.source().value_at(key), (true, Aggregation::OverHidden) => self.fold(std::slice::from_ref(key), func), (false, Aggregation::Exact) => { let members = expansion.members(); - let cells = members.iter().filter_map(|m| self.model.data.get(m)); - fold_groups(&[&Group::of(cells)], func) + let cells: Vec = members + .iter() + .filter_map(|m| self.model.source().value_at(m)) + .collect(); + fold_groups(&[&Group::of(&cells)], func) } (false, Aggregation::OverHidden) => self.fold(&expansion.members(), func), } } fn fold(&self, keys: &[CellKey], func: Reduce) -> Option { - self.shapes - .read(&self.model.data, keys, |groups| fold_groups(groups, func)) + self.shapes.read(self.model.source(), keys, |groups| { + fold_groups(groups, func) + }) } /// The items a stored cell may hold in `cat` to be named by `item`: that item, or a member @@ -134,7 +182,7 @@ impl Incremental { fn cells_at(&self, at: &CellKey) -> i64 { let keys = std::slice::from_ref(at); let cells = |groups: &[&Group]| groups.first().map_or(0, |g| g.cells()); - self.shapes.read(&self.model.data, keys, cells) + self.shapes.read(self.model.source(), keys, cells) } /// Cells at or under `at` that draw no hidden coordinate: all of them, less those with a @@ -190,7 +238,7 @@ impl Incremental { } /// Brings the formula cells up to date with an edit and the store changes it made. - fn refresh_formulas(&mut self, edit: &Edit, changes: &[CellChange]) { + fn refresh_formulas(&mut self, edit: Option<&Edit>, changes: &[CellChange]) { if self.fingerprint != self.fingerprint() { return self.rebuild_formulas(); } @@ -203,7 +251,7 @@ impl Incremental { self.clear_stem(stem); } let mut dirty = moved.dirty; - if let Edit::SetMeasureAgg { measure, .. } = edit { + if let Some(Edit::SetMeasureAgg { measure, .. }) = edit { dirty.extend(self.stems.carrying(measure)); if self.stems.reads_name(measure) { dirty.extend(self.stems.live().cloned()); @@ -343,8 +391,7 @@ impl Derived for Incremental { impl Maintained for Incremental { fn apply(&mut self, edit: &Edit) { edit.apply(&mut self.model); - let changes = self.model.data.drain_changes(); - self.shapes.apply(&cell_delta(&changes), &changes); - self.refresh_formulas(edit, &changes); + let changes = self.drain(); + self.absorb(Some(edit), &changes); } } diff --git a/crates/improvise-incremental/src/shapes.rs b/crates/improvise-incremental/src/shapes.rs index 4bad5cd3..5ac700be 100644 --- a/crates/improvise-incremental/src/shapes.rs +++ b/crates/improvise-incremental/src/shapes.rs @@ -3,7 +3,8 @@ use crate::group::Group; use crate::{CellDelta, DeltaValue}; -use improvise_core::model::cell::{CellChange, CellKey, CellValue, DataStore}; +use improvise_core::model::cell::{CellChange, CellKey, CellValue}; +use improvise_core::model::source::CellSource; use std::cell::{Cell, RefCell}; use std::collections::BTreeMap; @@ -111,28 +112,28 @@ impl Shape { (updates, sums) } - /// The shape of `store`'s cells, one pass. - fn of(store: &DataStore, cats: &[String]) -> (Shape, u64) { + /// The shape of `source`'s cells, one pass. + fn of(source: &dyn CellSource, cats: &[String]) -> (Shape, u64) { let mut shape = Shape { groups: BTreeMap::new(), }; let mut visited = 0; - for (key, value) in store.iter_cells() { + for (key, value) in source.cells(&[]) { visited += 1; let Some(at) = project(&key, cats) else { continue; }; let group = shape.groups.entry(at).or_default(); - group.apply(&DeltaValue::new(value), 1); + group.apply(&DeltaValue::new(&value), 1); if let CellValue::Number(x) = value { - group.add_number(*x, 1); + group.add_number(x, 1); } } (shape, visited) } } -/// Every shape a read has asked for. A shape is built from the store on its first read (a +/// Every shape a read has asked for. A shape is built from the source on its first read (a /// full pass) and fed every delta after. #[derive(Default)] pub struct Shapes { @@ -177,7 +178,7 @@ impl Shapes { /// shape first if no read has asked for it. A key with no group is left out. pub fn read( &self, - store: &DataStore, + source: &dyn CellSource, keys: &[CellKey], read: impl FnOnce(&[&Group]) -> R, ) -> R { @@ -192,7 +193,7 @@ impl Shapes { ); let mut built = self.built.borrow_mut(); let shape = built.entry(cats.clone()).or_insert_with(|| { - let (shape, visited) = Shape::of(store, &cats); + let (shape, visited) = Shape::of(source, &cats); self.bump(|m| { m.builds += 1; m.built_cells += visited; diff --git a/crates/improvise-incremental/tests/live_source.rs b/crates/improvise-incremental/tests/live_source.rs new file mode 100644 index 00000000..ba6a622a --- /dev/null +++ b/crates/improvise-incremental/tests/live_source.rs @@ -0,0 +1,213 @@ +//! `Incremental` over a source that reports its own changes (ADR 009 step 7, improvise-rnak). +//! Evidence: `context/test-evidence/improvise-incremental.md`. Counts, never time. + +mod support; + +use improvise_core::formula::ast::AggFunc; +use improvise_core::model::cell::{CellKey, CellValue}; +use improvise_core::model::source::ChangeSource; +use improvise_core::model::{Aggregation, Model}; +use improvise_incremental::{ + Derived, Eager, Edit, Maintained, Meter, differential_in, edit_probes, probes, same_value, +}; +use support::live::Fed; +use support::{corpus, edits_for, hand_model, key}; + +#[test] +fn a_live_source_matches_eager_over_the_corpus_and_its_edits() { + let (mut compared, mut files_done) = (0, Vec::::new()); + for (name, model, none, views) in corpus() { + let file = name.split(':').next().unwrap().to_string(); + if files_done.contains(&file) { + continue; + } + files_done.push(file); + let mut edits = edits_for(&model); + if model.data.iter_cells().count() > 40 { + let picked = [0, 2, 4].iter().filter_map(|&i| edits.get(i).cloned()); + edits = picked.collect(); + } + let candidate = Fed::new(&model, none.clone()); + let ran = differential_in(&model, &views, &none, candidate, &edits); + let report = ran.unwrap_or_else(|d| panic!("{name}: {d}")); + assert_eq!(report.steps, edits.len() + 1, "{name}"); + compared += report.probes; + } + assert!(compared > 10_000, "only {compared} answers were compared"); +} + +#[test] +fn changes_named_by_the_caller_are_maintained_as_the_drain_would() { + let (model, none) = (hand_model(), vec!["Region".to_string()]); + let mut eager = Eager::new(model.clone(), none.clone()); + let mut fed = Fed::new(&model, none); + let edits = [ + Edit::SetCell(key("East", "A", "Revenue"), CellValue::Number(99.0)), + Edit::SetCell(key("North", "A", "Revenue"), CellValue::Number(1.0)), + Edit::ClearCell(key("West", "B", "Cost")), + Edit::SetCell(key("East", "A", "Note"), CellValue::Text("z".into())), + ]; + let mut questions = probes(eager.model()); + let mut asked = 0; + for edit in &edits { + eager.apply(edit); + let (cell, value) = match edit { + Edit::SetCell(cell, value) => (cell, Some(value.clone())), + Edit::ClearCell(cell) => (cell, None), + _ => unreachable!("cell edits only"), + }; + fed.live.write(cell.clone(), value); + let changes = fed.live.drain_changes(); + assert_eq!(changes.len(), 1); + fed.inc.apply_changes(&changes); + questions.extend(probes(eager.model())); + questions.extend(edit_probes(edit)); + for probe in &questions { + let (want, got) = (probe.ask(&eager), probe.ask(&fed)); + assert!( + same_value(&want, &got), + "{probe:?}: eager {want:?} != fed {got:?}" + ); + asked += 1; + } + } + assert!(asked > 500, "only {asked} answers were compared"); +} + +#[test] +fn a_write_is_invisible_until_it_is_drained() { + let model = hand_model(); + let mut fed = Fed::new(&model, vec![]); + let cell = key("East", "A", "Revenue"); + let before = fed.inc.value(&cell, Aggregation::Exact); + assert_eq!(before, Some(CellValue::Number(10.0))); + fed.live.write(cell.clone(), Some(CellValue::Number(50.0))); + assert_eq!( + fed.inc.value(&cell, Aggregation::Exact), + before, + "not yet drained" + ); + fed.inc.sync(); + assert_eq!( + fed.inc.value(&cell, Aggregation::Exact), + Some(CellValue::Number(50.0)) + ); + fed.inc.sync(); + assert_eq!( + fed.inc.value(&cell, Aggregation::Exact), + Some(CellValue::Number(50.0)) + ); +} + +fn row(r: usize, p: usize, m: usize) -> CellKey { + CellKey::new(vec![ + ("Region".into(), format!("r{r}")), + ("Product".into(), format!("p{p}")), + ("Month".into(), format!("m{m}")), + ("_Measure".into(), "Revenue".into()), + ]) +} + +/// `n` regions by `n` products by 6 months of one measure, as a model with a profit-style formula. +fn table(n: usize) -> Model { + let mut model = Model::new("table"); + for (cat, prefix, count) in [("Region", "r", n), ("Product", "p", n), ("Month", "m", 6)] { + model.add_category(cat).unwrap(); + for i in 0..count { + model + .category_mut(cat) + .unwrap() + .add_item(format!("{prefix}{i}")); + } + } + model.category_mut("_Measure").unwrap().add_item("Revenue"); + model.measure_agg.insert("Revenue".into(), AggFunc::Max); + let mut seed = 1u64; + for (r, p, m) in (0..n).flat_map(|r| (0..n).flat_map(move |p| (0..6).map(move |m| (r, p, m)))) { + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + model.set_cell( + row(r, p, m), + CellValue::Number(((seed >> 33) % 1000) as f64), + ); + } + model +} + +fn since(before: Meter, after: Meter) -> Meter { + Meter { + updates: after.updates - before.updates, + sum_updates: after.sum_updates - before.sum_updates, + reads: after.reads - before.reads, + builds: after.builds - before.builds, + built_cells: after.built_cells - before.built_cells, + stems: after.stems - before.stems, + formula_cells: after.formula_cells - before.formula_cells, + rebuilds: after.rebuilds - before.rebuilds, + recomputes: after.recomputes - before.recomputes, + } +} + +fn region_partial() -> CellKey { + CellKey::new(vec![ + ("Region".into(), "r1".into()), + ("_Measure".into(), "Revenue".into()), + ]) +} + +/// What one reported change costs once a shape is built, and what the source served for it. +fn change_cost(n: usize) -> (Meter, u64) { + let mut fed = Fed::new(&table(n), vec![]); + fed.inc.raw_fold(®ion_partial()); + let (before, served) = (fed.inc.meter(), fed.live.snapshot_rows()); + assert_eq!( + served, + (n * n * 6) as u64, + "the build is one pass over the table" + ); + fed.live + .write(row(1, 2, 3), Some(CellValue::Number(2000.0))); + fed.inc.sync(); + let cost = since(before, fed.inc.meter()); + assert_eq!( + fed.inc.raw_fold(®ion_partial()), + Some(CellValue::Number(2000.0)), + "the new maximum comes from the change" + ); + (cost, fed.live.snapshot_rows() - served) +} + +#[test] +fn the_cost_of_a_reported_change_does_not_depend_on_the_size_of_the_table() { + let (small, small_rows) = change_cost(6); + let (large, large_rows) = change_cost(12); + assert_eq!( + (small.updates, small.sum_updates), + (2, 2), + "retract and insert" + ); + assert_eq!(small, large, "216 cells and 864 cells cost the same"); + assert_eq!( + (small.builds, small.built_cells), + (0, 0), + "no shape is rebuilt" + ); + assert_eq!( + (small_rows, large_rows), + (0, 0), + "the source serves no snapshot for a change" + ); +} + +#[test] +fn an_overwrite_by_the_same_value_moves_nothing() { + let mut fed = Fed::new(&hand_model(), vec![]); + let cell = key("East", "A", "Revenue"); + let revenue = CellKey::new(vec![("_Measure".into(), "Revenue".into())]); + let before = fed.inc.raw_fold(&revenue); + fed.live.write(cell, Some(CellValue::Number(10.0))); + fed.inc.sync(); + assert_eq!(fed.inc.raw_fold(&revenue), before); + assert_eq!(before, Some(CellValue::Number(23.0))); +} diff --git a/crates/improvise-incremental/tests/random_live.proptest-regressions b/crates/improvise-incremental/tests/random_live.proptest-regressions new file mode 100644 index 00000000..5698220b --- /dev/null +++ b/crates/improvise-incremental/tests/random_live.proptest-regressions @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 78a76cbc766ceb9d87768ca4fe518dc0773289e681621477b5f2fca6d15e36c9 # shrinks to edits = [SetCell(CellKey([("Product", "A"), ("Region", "East"), ("_Measure", "Revenue")]), Number(0.0))] diff --git a/crates/improvise-incremental/tests/random_live.rs b/crates/improvise-incremental/tests/random_live.rs new file mode 100644 index 00000000..82330922 --- /dev/null +++ b/crates/improvise-incremental/tests/random_live.rs @@ -0,0 +1,22 @@ +//! `Incremental` over a live source against `Eager` over random formula and cell edits. +//! Evidence: `context/test-evidence/improvise-incremental.md`. + +mod support; + +use improvise_incremental::differential_in; +use proptest::prelude::*; +use support::live::Fed; +use support::{any_formula_edit, formula_workbook}; + +proptest! { + #[test] + fn a_live_source_matches_eager_over_random_formula_and_cell_edits( + edits in prop::collection::vec(any_formula_edit(), 0..3), + ) { + let wb = formula_workbook(); + let none = vec!["Region".to_string()]; + let candidate = Fed::new(&wb.model, none.clone()); + let ran = differential_in(&wb.model, &[], &none, candidate, &edits); + prop_assert!(ran.is_ok(), "{}", ran.unwrap_err()); + } +} diff --git a/crates/improvise-incremental/tests/support/live.rs b/crates/improvise-incremental/tests/support/live.rs new file mode 100644 index 00000000..8d6d2259 --- /dev/null +++ b/crates/improvise-incremental/tests/support/live.rs @@ -0,0 +1,156 @@ +//! A live table: a scripted source that reports its changes, the way a database would. +//! Its rows are those as of the last drain; a write is pending until then. +#![allow(dead_code)] + +use improvise_core::model::cell::{CellChange, CellKey, CellValue, DataStore}; +use improvise_core::model::source::{CellSource, ChangeSource, Select, Summary}; +use improvise_core::model::{Aggregation, Model}; +use improvise_incremental::{Derived, Edit, Incremental, Maintained, Reading}; +use std::sync::{Arc, Mutex}; + +#[derive(Default)] +struct State { + /// What a read sees: the rows as of the last drain. + visible: DataStore, + /// The rows with every pending write applied, to say what a write changed. + latest: DataStore, + pending: Vec, + /// Rows handed out by a snapshot read (`cells` with no partial). + snapshot_rows: u64, +} + +#[derive(Default)] +pub struct LiveTable { + state: Mutex, +} + +impl LiveTable { + /// A table already holding `rows`, with nothing pending. + pub fn of(rows: impl IntoIterator) -> Self { + let mut state = State::default(); + for (key, value) in rows { + state.visible.set(key.clone(), value.clone()); + state.latest.set(key, value); + } + LiveTable { + state: Mutex::new(state), + } + } + + /// Writes `value` at `key` (or removes it), as a database user would: visible after the next drain. + pub fn write(&self, key: CellKey, value: Option) { + let mut state = self.state.lock().unwrap(); + let old = state.latest.get(&key).cloned(); + match &value { + Some(v) => state.latest.set(key.clone(), v.clone()), + None if old.is_none() => return, + None => state.latest.remove(&key), + } + state.pending.push(CellChange { + key, + old, + new: value, + }); + } + + /// How many rows snapshot reads have handed out so far. + pub fn snapshot_rows(&self) -> u64 { + self.state.lock().unwrap().snapshot_rows + } +} + +impl CellSource for LiveTable { + fn value_at(&self, key: &CellKey) -> Option { + self.state.lock().unwrap().visible.value_at(key) + } + + fn summarize(&self, select: &Select) -> Summary { + self.state.lock().unwrap().visible.summarize(select) + } + + fn keys<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + let state = self.state.lock().unwrap(); + let keys: Vec = state.visible.keys(partial).collect(); + Box::new(keys.into_iter()) + } + + fn cells<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + let mut state = self.state.lock().unwrap(); + let cells: Vec<_> = state.visible.cells(partial).collect(); + if partial.is_empty() { + state.snapshot_rows += cells.len() as u64; + } + Box::new(cells.into_iter()) + } +} + +impl ChangeSource for LiveTable { + fn drain_changes(&self) -> Vec { + let mut state = self.state.lock().unwrap(); + let changes = std::mem::take(&mut state.pending); + for change in &changes { + match &change.new { + Some(value) => state.visible.set(change.key.clone(), value.clone()), + None => state.visible.remove(&change.key), + } + } + changes + } +} + +/// An `Incremental` over a live table that holds the model's cells; the model keeps no store. +/// A cell edit is a write to the table, the way another client's would be, followed by a drain. +pub struct Fed { + pub inc: Incremental, + pub live: Arc, +} + +impl Fed { + pub fn new(model: &Model, none_cats: Vec) -> Fed { + let rows = model.data.iter_cells().map(|(k, v)| (k, v.clone())); + let live = Arc::new(LiveTable::of(rows)); + let mut bare = model.clone(); + bare.data = DataStore::new(); + let inc = Incremental::over(bare, live.clone(), none_cats); + Fed { inc, live } + } +} + +impl Derived for Fed { + fn value(&self, key: &CellKey, agg: Aggregation) -> Option { + self.inc.value(key, agg) + } + + fn raw_fold(&self, partial: &CellKey) -> Option { + self.inc.raw_fold(partial) + } + + fn covered(&self, key: &CellKey, reading: &Reading) -> Option { + self.inc.covered(key, reading) + } + + fn total(&self, key: &CellKey, over: &[String], reading: &Reading) -> Option { + self.inc.total(key, over, reading) + } + + fn occupancy(&self, partial: &CellKey, reading: &Reading) -> usize { + self.inc.occupancy(partial, reading) + } +} + +impl Maintained for Fed { + fn apply(&mut self, edit: &Edit) { + match edit { + Edit::SetCell(key, value) => self.live.write(key.clone(), Some(value.clone())), + Edit::ClearCell(key) => self.live.write(key.clone(), None), + _ => return self.inc.apply(edit), + } + self.inc.sync(); + } +} diff --git a/crates/improvise-incremental/tests/support/mod.rs b/crates/improvise-incremental/tests/support/mod.rs index 0ba3d060..db732221 100644 --- a/crates/improvise-incremental/tests/support/mod.rs +++ b/crates/improvise-incremental/tests/support/mod.rs @@ -1,6 +1,8 @@ //! Fixtures the integration tests share: the shipped corpus and the hand-built models. #![allow(dead_code)] +pub mod live; + use improvise_core::formula::ast::AggFunc; use improvise_core::formula::parse_formula; use improvise_core::model::Aggregation; From 6bc102f37874cacab105a5f9cb07eaf7de201e06 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:13:15 -0700 Subject: [PATCH 05/14] chore(incremental): drop a proptest seed an injected fault left behind (improvise-rnak) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../tests/random_live.proptest-regressions | 7 ------- 1 file changed, 7 deletions(-) delete mode 100644 crates/improvise-incremental/tests/random_live.proptest-regressions diff --git a/crates/improvise-incremental/tests/random_live.proptest-regressions b/crates/improvise-incremental/tests/random_live.proptest-regressions deleted file mode 100644 index 5698220b..00000000 --- a/crates/improvise-incremental/tests/random_live.proptest-regressions +++ /dev/null @@ -1,7 +0,0 @@ -# Seeds for failure cases proptest has generated in the past. It is -# automatically read and these particular cases re-run before any -# novel cases are generated. -# -# It is recommended to check this file in to source control so that -# everyone who runs the test benefits from these saved cases. -cc 78a76cbc766ceb9d87768ca4fe518dc0773289e681621477b5f2fca6d15e36c9 # shrinks to edits = [SetCell(CellKey([("Product", "A"), ("Region", "East"), ("_Measure", "Revenue")]), Number(0.0))] From 6bda8a479bb1fe895607515817de4ad53a09725c Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:19:56 -0700 Subject: [PATCH 06/14] feat(incremental): derived values over a source that answers aggregate queries (improvise-rnak) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- Cargo.nix | 5 + context/code/improvise-incremental.md | 47 ++++ context/code/source.md | 16 ++ context/test-evidence/core.md | 21 ++ .../test-evidence/improvise-incremental.md | 57 +++++ crates/improvise-core/src/model/source.rs | 139 +++++++++++ crates/improvise-core/src/model/types.rs | 12 +- crates/improvise-incremental/Cargo.toml | 2 + crates/improvise-incremental/src/lib.rs | 2 + crates/improvise-incremental/src/pushdown.rs | 104 ++++++++ .../improvise-incremental/tests/pushdown.rs | 197 +++++++++++++++ .../tests/random_pushdown.rs | 22 ++ .../tests/support/live.rs | 8 + .../tests/support/mod.rs | 1 + .../tests/support/remote.rs | 226 ++++++++++++++++++ 15 files changed, 850 insertions(+), 9 deletions(-) create mode 100644 crates/improvise-incremental/src/pushdown.rs create mode 100644 crates/improvise-incremental/tests/pushdown.rs create mode 100644 crates/improvise-incremental/tests/random_pushdown.rs create mode 100644 crates/improvise-incremental/tests/support/remote.rs diff --git a/Cargo.nix b/Cargo.nix index cacfd3f9..6db877fd 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -3095,6 +3095,11 @@ rec { } ]; devDependencies = [ + { + name = "improvise-core"; + packageId = "improvise-core"; + features = [ "test-support" ]; + } { name = "improvise-io"; packageId = "improvise-io"; diff --git a/context/code/improvise-incremental.md b/context/code/improvise-incremental.md index 0791e1de..c8021050 100644 --- a/context/code/improvise-incremental.md +++ b/context/code/improvise-incremental.md @@ -427,3 +427,50 @@ from the source's folds, never pushed down; `Derived` stays in this crate. `tests/support/live.rs` holds `LiveTable`, the scripted source (an indexed store behind a lock, with a pending list and a counter of snapshot rows) and `Fed`, an `Incremental` over one that a differential can drive: a cell edit becomes a write to the table followed by `sync`. + +### Query pushdown (`Pushdown`) + +For a source that answers aggregate queries and reports nothing. `Pushdown::new(definition, source, +none_cats)` puts the model over the source (`Model::over`) and keeps nothing else: no shapes, no +change log, no stems. These are the readings taken. + +1. **`AggregateQuery` is `CellSource`; there are two traits, not three.** The brief's `AggregateQuery` + (`fold`, `count`) is the trait core already has: `summarize` is the fold, `count` the count. `Derived` + stays here (user ruling) and stays a different thing: the values a view draws, which a backend + builds from a source's aggregates and its own state. A source implements `CellSource` and a backend + implements `Derived`. +2. **Folds are queries, formula cells are computed in RAM.** `value`, `raw_fold`, `covered` and `total` are + the model's evaluator over `Model::source()`, so a fold is one `summarize` and the rest is arithmetic + on the answers. The formula cache is the one thing `Pushdown` holds. That is the user's ruling + (2026-10-10), and it makes the evaluator hook the only change to core's evaluation. +3. **Staleness is `refresh()`.** A stored cell or a fold is current at the moment of the read. A formula + cell is the cache's value until `refresh` recomputes it from the source as it now stands, which + costs one `distinct_without` per formula and the folds each formula reads. When to refresh is the + host's (ADR 003's "polled, triggered, or explicitly stale"); nothing here schedules it. A + delivery layer that wants notices from a pushdown-backed shape has no moved keys to name, so it + marks them "correct" (ADR 009 clause 9), which is a bead (below). +4. **Occupancy is one `count`.** A group in the partial becomes a span over its own name and its + members; the view's hidden pairs go as `excluding`. `Eager::occupancy` lists every key to say the + same thing. +5. **`define(edit)` takes a formula or an aggregation edit and refreshes.** A cell edit is the source's: + `Model::set_cell` refuses it in a debug build. +6. **What is and is not proven.** The tests run against `Remote` (`tests/support/remote.rs`), a scanning + in-memory table with counters, that shares no evaluator with `Eager` and answers queries over rows. + Without a real SQL engine only the **contract** is proven: that a source answering these six + queries as specified gives `Eager`'s answers, and what each read costs in queries. Nothing here + shows that SQLite can answer `summarize` exactly (an exact sum is not what its `SUM` returns: + `context/code/source.md`), or what a query costs. The first real source is improvise-rucf, still + deferred. + +### What step 7 did not predict + +- **The hook came before the change feed**, not after (a fed backend's formulas are evaluated over the + source; see `context/code/source.md`). +- **`TableSource` and `LiveTable` found nothing in `Incremental`**; the corpus and the random edits pass + over a source on the first run. What the tests found was in the fixtures: a rival that read the owned + store for the stored fallback of a filtered formula passed until a cell was stored where the `WHERE` + fails, and a rival that forgot a group's own name as an item passed until a cell was stored under + one. +- **The two-second limit decided the shape of the tests** more than the design did: the corpus + differential over a scanning source is 2.5s untrimmed, so it follows `corpus_eager`'s trimming, and + each random test is its own binary. diff --git a/context/code/source.md b/context/code/source.md index 7ed4d1d1..a7e81176 100644 --- a/context/code/source.md +++ b/context/code/source.md @@ -14,6 +14,8 @@ they leave a choice. | `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 | +| `count(select, excluding)` | how many cells a select names, leaving out those holding a hidden pair: occupancy | +| `distinct_without(cats)` | the cell keys with `cats` removed, each once, first seen first: the stems formulas are evaluated at | 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, @@ -96,3 +98,17 @@ now plus the changes drained later counts no change twice or not at all. Why, an uses it: `context/code/improvise-incremental.md`, step 7. `DataStore` is not a `ChangeSource`: its log is `&mut` and is drained by the owner in the step that made the edit. A `SharedFeed` is the `Arc` a backend and a model share. + +## Pushdown needs two more queries + +`count` and `distinct_without` joined the trait for `Pushdown` (improvise-rnak). Both are what a query +engine answers without sending rows (`COUNT(*) ... WHERE`, `SELECT DISTINCT`), and each replaces a +read that would have transferred every key: `Eager::occupancy` lists all keys, and +`recompute_formulas` walked every cell to find its stems. `recompute_formulas` now asks +`distinct_without(target and None categories)` once per formula, and the order it returns is +the order the cells were first seen in, so the stems are inserted in the order they always were +(the passes of a fixed point visit stems in that order). A source with no order of its own must +fix one, or the passes of a document whose stems read each other may differ between refreshes. + +`Select::admits` is the predicate of a select over one key, for a source that must test keys itself +(`DataStore::count`). `TableSource` does not use it: its own `holds` is the independent second reading. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 69226c41..bd52dbcc 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -2380,3 +2380,24 @@ branch reads the owned store; it fails here. `set_cell` on a model over a source panics in a debug build, naming the reason. Without the assert the write goes to the owned store and nothing reads it. + +### model::source::tests::the_store_and_a_scanned_table_count_every_select_alike + +improvise-rnak. `count(select, excluding)` on the store and on a scanning table, over every select of +the fixture, at both widths, hiding none, each of four items and each pair of them: 31,680 counts, 3,370 +of them not zero (a count that was zero on both sides would pass). Rivals, each failing it: the count +ignores the excluded pairs (also fails `hiding_an_item_removes_exactly_the_cells_that_hold_it`), ignores the +spans, and ignores `exact`. + +### model::source::tests::hiding_an_item_removes_exactly_the_cells_that_hold_it + +The hand-checked half: all rows, all but those holding `R=E`, and a select naming a hidden item is +zero. A count that agreed with a wrong scan on both sides would pass the test above and fail this. + +### model::source::tests::the_store_and_a_scanned_table_list_the_same_stems + +`distinct_without` on the store and on a scanning table, for every subset of three categories and one +that does not exist, equal and in the same order, each stem once. Rivals, each failing it by name: +the stems not deduplicated, sorted instead of first seen (this is the only test that sees it: the +evaluator's results do not depend on stem order for the fixtures), and the dropped categories kept (also +fails several evaluator tests). diff --git a/context/test-evidence/improvise-incremental.md b/context/test-evidence/improvise-incremental.md index 61011e72..8578822e 100644 --- a/context/test-evidence/improvise-incremental.md +++ b/context/test-evidence/improvise-incremental.md @@ -842,3 +842,60 @@ the model not over the feed fails it with the others. `Fed` against `Eager` over random formula and cell edits on the formula fixture (no views, so the binary stays under two seconds). Rivals: `sync` drains nothing, the model not over the feed, a shape built from an empty store and the stems from the owned store all fail it. + +### a_pushdown_matches_eager_over_the_shipped_documents + +improvise-rnak. `Pushdown` over `Remote` (a scanning table that counts its queries; the model keeps no +cells) against `Eager`, over the shipped documents, their views and `edits_for` (trimmed as +`corpus_eager` is). Rivals, each failing it by name: the occupancy forgetting the hidden items; the +model not over the source; a raw fold answering for a formula target; a covered read covering every +member; a total covering every member; the engine ignoring `exact`; a refresh that does nothing; a +definition edit that does not refresh. + +### a_pushdown_matches_eager_on_hidden_items_and_collapsed_groups_under_every_aggregation + +The hidden and the counted fixtures, drawn, under each of five aggregations, with a NaN, a `-0.0` and +a text written to a hidden cell and a cell stored under the group's own name. The last is why the +rival "occupancy forgets the group's own name" is caught: before that edit, it passed everywhere. +Rivals: the occupancy treating a group as an item; the same forgetting the group's name; the occupancy +forgetting the hidden items; covered and total covering everything. + +### a_pushdown_matches_eager_on_formulas_groups_and_wide_keys + +The ratio fixture (a formula over two measures), the hand model under every aggregation of two +measures, a grouped model and the six-category wide model. Rivals: a refresh that does nothing, a +definition edit that does not refresh, a raw fold of a formula target, the engine ignoring `exact`. + +### a_fold_is_one_query_and_sends_no_row + +A raw fold, a collapsed group's covered read and a column total are three `summarize` calls between +them and no row sent back by the table (`rows_sent` unchanged); an exact read is one lookup. Rival: +the model not over the source (the reads then never reach the table). + +### occupancy_is_one_count_whatever_the_groups_and_hidden_items + +The Coastal-by-A occupancy of the hidden fixture (East hidden, West holding A) is 1, in one `count`, +with no row sent. Rivals: the hidden items forgotten, a group treated as an item. + +### formula_cells_are_computed_from_the_distinct_stems_not_from_every_cell + +Refreshing the hand model's one formula asks for stems once and gets two rows back (Product A and B) +from twelve cells, lists no keys and no cells, and still queries the folds the formula reads. +Rivals: a refresh that does nothing, the model not over the source. + +### a_stored_cell_is_current_and_a_formula_cell_waits_for_a_refresh + +The staleness contract of `Pushdown`. A write by another client is seen by the next exact read (50) and +not by the cached formula cell (9) until `refresh` (49). Rival: a refresh that does nothing. + +### a_source_that_forgets_the_hidden_items_is_caught_as_a_divergence + +The check of the checker, the user's ruling about faults: `IgnoresHidden` is a source whose `count` +drops the hidden pairs. The differential must report a `Divergence` and the probe must be an +occupancy; a sound source over the same fixture must pass. + +### a_pushdown_matches_eager_over_random_formula_and_cell_edits + +`Pushdown` over `Remote` against `Eager` over random formula and cell edits on the formula fixture +(cycles, text, NaN and `-0.0` among the values), in its own binary. Rival: a refresh that does +nothing. diff --git a/crates/improvise-core/src/model/source.rs b/crates/improvise-core/src/model/source.rs index 5c3fd8a0..661ecafc 100644 --- a/crates/improvise-core/src/model/source.rs +++ b/crates/improvise-core/src/model/source.rs @@ -24,6 +24,16 @@ impl<'a> Select<'a> { } } + /// Whether the cell at `key` is one this select names. + pub fn admits(&self, key: &CellKey) -> bool { + let spanned = |(cat, items): &(&str, Vec<&str>)| { + key.get(cat).is_some_and(|held| items.contains(&held)) + }; + key.matches_partial(self.base) + && self.spans.iter().all(spanned) + && (!self.exact || key.0.len() == self.base.len() + self.spans.len()) + } + /// Every cell containing `base` and one listed item of each span. pub fn spanning( base: &'a [(String, String)], @@ -200,6 +210,20 @@ pub trait CellSource { &'a self, partial: &'a [(String, String)], ) -> Box + 'a>; + + /// How many cells `select` names, leaving out those holding any of the `excluding` pairs. + fn count(&self, select: &Select, excluding: &[(String, String)]) -> usize; + + /// The key of every cell with the categories in `without` removed, each once, in the order + /// first seen: the stems a formula is evaluated at, as `SELECT DISTINCT` gives them. + fn distinct_without(&self, without: &[String]) -> Vec; +} + +/// Whether the cell at `key` holds any of the `pairs`. +fn holds_any(key: &CellKey, pairs: &[(String, String)]) -> bool { + pairs + .iter() + .any(|(cat, item)| key.get(cat) == Some(item.as_str())) } impl CellSource for DataStore { @@ -236,6 +260,19 @@ impl CellSource for DataStore { ), } } + + fn count(&self, select: &Select, excluding: &[(String, String)]) -> usize { + let named = self.keys(select.base).filter(|key| select.admits(key)); + named.filter(|key| !holds_any(key, excluding)).count() + } + + fn distinct_without(&self, without: &[String]) -> Vec { + let mut seen = std::collections::HashSet::new(); + let stems = self + .iter_cells() + .map(|(key, _)| without.iter().fold(key, |k, cat| k.without(cat))); + stems.filter(|stem| seen.insert(stem.clone())).collect() + } } /// A table of rows read by scanning, the way a query engine with no index would. A second @@ -291,6 +328,28 @@ impl CellSource for TableSource { .cloned(), ) } + + fn count(&self, select: &Select, excluding: &[(String, String)]) -> usize { + let has = |key: &CellKey, (cat, item): &(String, String)| { + key.0.iter().any(|(c, i)| c == cat && i == item) + }; + let named = self.rows.iter().filter(|(key, _)| Self::holds(key, select)); + named + .filter(|(key, _)| !excluding.iter().any(|pair| has(key, pair))) + .count() + } + + fn distinct_without(&self, without: &[String]) -> Vec { + let mut stems: Vec = Vec::new(); + for (key, _) in &self.rows { + let kept = key.0.iter().filter(|(c, _)| !without.contains(c)); + let stem = CellKey::new(kept.cloned().collect()); + if !stems.contains(&stem) { + stems.push(stem); + } + } + stems + } } #[cfg(test)] @@ -569,4 +628,84 @@ mod tests { "no cell is keyed by R=E alone" ); } + + /// Every way to hide items of the fixture: none, each item of each category, and two at once. + fn hidden_sets() -> Vec> { + let items = [("R", "E"), ("R", "W"), ("P", "A"), ("_Measure", "Note")]; + let mut sets = vec![vec![]]; + for (i, a) in items.iter().enumerate() { + sets.push(vec![pair(a.0, a.1)]); + for b in &items[i + 1..] { + sets.push(vec![pair(a.0, a.1), pair(b.0, b.1)]); + } + } + sets + } + + #[test] + fn the_store_and_a_scanned_table_count_every_select_alike() { + let rows = rows(); + let (store, table) = (store_of(&rows), TableSource { rows: rows.clone() }); + let (mut asked, mut nonzero) = (0, 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 hidden in hidden_sets() { + let (a, b) = (store.count(&select, &hidden), table.count(&select, &hidden)); + assert_eq!(a, b, "{base:?} {spans:?} exact={exact} hiding {hidden:?}"); + asked += 1; + nonzero += usize::from(a > 0); + } + } + } + assert!(asked > 20_000, "the enumeration shrank to {asked}"); + assert!(nonzero > 2_000, "{nonzero} of {asked} counts were not zero"); + } + + #[test] + fn hiding_an_item_removes_exactly_the_cells_that_hold_it() { + let rows = rows(); + let store = store_of(&rows); + let none = Select::matching(&[]); + assert_eq!(store.count(&none, &[]), rows.len()); + let east = [pair("R", "E")]; + let held = rows.iter().filter(|(k, _)| k.get("R") == Some("E")).count(); + assert_eq!(store.count(&none, &east), rows.len() - held); + assert_eq!( + store.count(&Select::matching(&east), &east), + 0, + "a named hidden item matches nothing" + ); + } + + #[test] + fn the_store_and_a_scanned_table_list_the_same_stems() { + let rows = rows(); + let (store, table) = (store_of(&rows), TableSource { rows: rows.clone() }); + let cats = ["R", "P", "_Measure", "Nowhere"].map(String::from); + let mut asked = 0; + for mask in 0..(1u32 << cats.len()) { + let without: Vec = cats + .iter() + .enumerate() + .filter(|(i, _)| mask >> i & 1 == 1) + .map(|(_, c)| c.clone()) + .collect(); + let (a, b) = ( + store.distinct_without(&without), + table.distinct_without(&without), + ); + assert_eq!(a, b, "without {without:?}, in first-seen order"); + let mut sorted = a.clone(); + sorted.sort(); + sorted.dedup(); + assert_eq!(sorted.len(), a.len(), "each stem once"); + asked += a.len(); + } + assert!(asked > 40, "only {asked} stems were compared"); + } } diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index d9c161e8..4b128590 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -1027,15 +1027,9 @@ 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.source().keys(&[]) { - let stem = key.without(target_cat); - // Strip none_cats too: the grid queries without them. - let mut stripped = stem; - for nc in none_cats { - stripped = stripped.without(nc); - } - stems.insert(stripped); - } + // The target and the none_cats go: the grid queries without them. + let without = [std::slice::from_ref(target_cat), none_cats].concat(); + stems.extend(self.source().distinct_without(&without)); } // Fixed-point iteration. diff --git a/crates/improvise-incremental/Cargo.toml b/crates/improvise-incremental/Cargo.toml index 20d49e06..f022965c 100644 --- a/crates/improvise-incremental/Cargo.toml +++ b/crates/improvise-incremental/Cargo.toml @@ -13,5 +13,7 @@ numtower = { path = "../numtower" } zset = { path = "../zset" } [dev-dependencies] +# `TableSource`, the scanning source the pushdown tests are built on. +improvise-core = { path = "../improvise-core", features = ["test-support"] } improvise-io = { path = "../improvise-io" } proptest = "1" diff --git a/crates/improvise-incremental/src/lib.rs b/crates/improvise-incremental/src/lib.rs index 8fddf659..ae6b9d70 100644 --- a/crates/improvise-incremental/src/lib.rs +++ b/crates/improvise-incremental/src/lib.rs @@ -9,6 +9,7 @@ mod formulas; mod group; mod harness; mod incremental; +mod pushdown; mod reading; mod shapes; @@ -20,6 +21,7 @@ pub use harness::{ drawn_probes, edit_probes, probes, same_value, }; pub use incremental::{Evaluation, Incremental}; +pub use pushdown::Pushdown; pub use reading::Reading; pub use shapes::Meter; diff --git a/crates/improvise-incremental/src/pushdown.rs b/crates/improvise-incremental/src/pushdown.rs new file mode 100644 index 00000000..90ef6f67 --- /dev/null +++ b/crates/improvise-incremental/src/pushdown.rs @@ -0,0 +1,104 @@ +//! Derived values over a source that answers aggregate queries and reports no changes. +//! Reasoning: `context/code/improvise-incremental.md`, step 7. + +use crate::{Derived, Edit, Reading}; +use improvise_core::model::cell::{CellKey, CellValue}; +use improvise_core::model::source::{CellSource, Select}; +use improvise_core::model::{Aggregation, Model}; +use std::sync::Arc; + +/// A model whose cells are in `source`: a fold is a query, the formula cells are computed in RAM +/// by the model's own evaluator from those folds, and nothing is kept per shape or per change. +pub struct Pushdown { + model: Model, + none_cats: Vec, +} + +impl Pushdown { + /// `definition` supplies the categories, formulas and aggregations; its own store is not read. + pub fn new( + definition: Model, + source: Arc, + none_cats: Vec, + ) -> Self { + let mut backend = Pushdown { + model: definition.over(source), + none_cats, + }; + backend.refresh(); + backend + } + + pub fn model(&self) -> &Model { + &self.model + } + + /// Computes the formula cells again from the source as it now stands. A read of a stored + /// cell or a fold is a query and always current; the formula cache is the one thing held. + pub fn refresh(&mut self) { + self.model.recompute_formulas(&self.none_cats); + } + + /// A change to the definition: a formula or an aggregation. Cells belong to the source. + pub fn define(&mut self, edit: &Edit) { + edit.apply(&mut self.model); + self.refresh(); + } + + /// The items a stored cell may hold in `cat` to be named by `item`, when `item` names a group. + fn members(&self, cat: &str, item: &str) -> Vec { + let group = self + .model + .category(cat) + .filter(|c| !c.items.contains_key(item)); + let members = group.map(|c| c.items_in_group(item)).unwrap_or_default(); + match members.is_empty() { + true => Vec::new(), + false => std::iter::once(item) + .chain(members) + .map(String::from) + .collect(), + } + } +} + +impl Derived for Pushdown { + fn value(&self, key: &CellKey, agg: Aggregation) -> Option { + self.model.evaluate_aggregated(key, agg) + } + + fn raw_fold(&self, partial: &CellKey) -> Option { + match self.model.formula_for(partial) { + Some(_) => None, + None => self.value(partial, Aggregation::OverHidden), + } + } + + fn covered(&self, key: &CellKey, reading: &Reading) -> Option { + let covers = |cat: &str, item: &str| reading.covers(cat, item); + self.model.evaluate_covering(key, reading.agg, &covers) + } + + fn total(&self, key: &CellKey, over: &[String], reading: &Reading) -> Option { + let covers = |cat: &str, item: &str| reading.covers(cat, item); + self.model + .evaluate_spanning(key, over, reading.agg, &covers) + } + + fn occupancy(&self, partial: &CellKey, reading: &Reading) -> usize { + let (mut base, mut held) = (Vec::new(), Vec::new()); + for (cat, item) in &partial.0 { + match self.members(cat, item) { + members if members.is_empty() => base.push((cat.clone(), item.clone())), + members => held.push((cat.clone(), members)), + } + } + let spans: Vec<(&str, Vec<&str>)> = held + .iter() + .map(|(cat, items)| (cat.as_str(), items.iter().map(String::as_str).collect())) + .collect(); + let hidden: Vec<(String, String)> = reading.hidden().cloned().collect(); + let select = Select::spanning(&base, &spans, false); + self.model.source().count(&select, &hidden) + } +} diff --git a/crates/improvise-incremental/tests/pushdown.rs b/crates/improvise-incremental/tests/pushdown.rs new file mode 100644 index 00000000..2b4eaea0 --- /dev/null +++ b/crates/improvise-incremental/tests/pushdown.rs @@ -0,0 +1,197 @@ +//! `Pushdown` over a remote table against `Eager`: values, hidden items, groups, formulas. +//! Without a real SQL engine only the contract is proven: `context/code/improvise-incremental.md`. + +mod support; + +use improvise_core::formula::ast::AggFunc; +use improvise_core::model::cell::{CellKey, CellValue}; +use improvise_core::model::{Aggregation, Model}; +use improvise_core::view::View; +use improvise_incremental::{Derived, Edit, Probe, Reading, differential_in}; +use support::remote::{IgnoresHidden, Pushed}; +use support::{ + corpus, edits_for, grouped_model, hand_model, hidden_counted_workbook, hidden_workbook, key, + none_of, ratio_workbook, wide_workbook, +}; + +fn run(model: &Model, views: &[View], none: &[String], edits: &[Edit]) -> usize { + let candidate = Pushed::new(model, none.to_vec()); + match differential_in(model, views, none, candidate, edits) { + Ok(report) => report.probes, + Err(d) => panic!("{d}"), + } +} + +fn agg_edits(measure: &str) -> Vec { + let all = [ + AggFunc::Count, + AggFunc::Min, + AggFunc::Max, + AggFunc::Avg, + AggFunc::Sum, + ]; + all.into_iter() + .map(|func| Edit::SetMeasureAgg { + measure: measure.into(), + func, + }) + .collect() +} + +#[test] +fn a_pushdown_matches_eager_over_the_shipped_documents() { + let (mut compared, mut files_done) = (0, Vec::::new()); + for (name, model, none, views) in corpus() { + let file = name.split(':').next().unwrap().to_string(); + if files_done.contains(&file) { + continue; + } + files_done.push(file); + let mut edits = edits_for(&model); + if model.data.iter_cells().count() > 40 { + let picked = [0, 2, 4].iter().filter_map(|&i| edits.get(i).cloned()); + edits = picked.collect(); + } + compared += run(&model, &views, &none, &edits); + } + assert!(compared > 10_000, "only {compared} answers were compared"); +} + +#[test] +fn a_pushdown_matches_eager_on_hidden_items_and_collapsed_groups_under_every_aggregation() { + for wb in [hidden_workbook(), hidden_counted_workbook()] { + let views: Vec = wb.views.values().cloned().collect(); + let none = none_of(&wb); + let mut edits = edits_for(&wb.model); + edits.extend(agg_edits("Revenue")); + let hidden = key("East", "A", "Revenue"); + edits.push(Edit::SetCell(hidden.clone(), CellValue::Number(f64::NAN))); + edits.push(Edit::SetCell(hidden.clone(), CellValue::Number(-0.0))); + edits.push(Edit::SetCell(hidden, CellValue::Text("t".into()))); + // A cell stored under the group's own name is a cell its fold and its count both see. + let named = key("Coastal", "B", "Revenue"); + edits.push(Edit::SetCell(named, CellValue::Number(4.0))); + assert!(run(&wb.model, &views, &none, &edits) > 1_000); + } +} + +#[test] +fn a_pushdown_matches_eager_on_formulas_groups_and_wide_keys() { + let ratio = ratio_workbook(); + let views: Vec = ratio.views.values().cloned().collect(); + let mut edits = edits_for(&ratio.model); + edits.extend(agg_edits("Revenue")); + run(&ratio.model, &views, &none_of(&ratio), &edits); + let hand = hand_model(); + let mut edits = agg_edits("Revenue"); + edits.extend(agg_edits("Note")); + run(&hand, &[], &["Region".to_string()], &edits); + run(&grouped_model(), &[], &[], &edits_for(&grouped_model())); + let wide = wide_workbook(); + let views: Vec = wide.views.values().cloned().collect(); + run( + &wide.model, + &views, + &none_of(&wide), + &edits_for(&wide.model), + ); +} + +#[test] +fn a_fold_is_one_query_and_sends_no_row() { + let wb = hidden_counted_workbook(); + let none = none_of(&wb); + let pushed = Pushed::new(&wb.model, none); + let at = |name: &str| key("Inland", name, "Revenue"); + let before = pushed.remote.asked(); + pushed.pd.raw_fold(&CellKey::new(vec![ + ("Product".into(), "A".into()), + ("_Measure".into(), "Revenue".into()), + ])); + let reading = Reading::of_view(wb.active_view(), &none_of(&wb)); + let coastal = key("Coastal", "A", "Revenue"); + pushed.pd.covered(&coastal, &reading); + let total = key("Inland", "A", "Revenue").without("Region"); + pushed.pd.total(&total, &["Region".to_string()], &reading); + let spent = pushed.remote.asked(); + assert_eq!(spent.summaries - before.summaries, 3, "one query per fold"); + assert_eq!(spent.rows_sent, before.rows_sent, "and no row sent back"); + let one = pushed.pd.value(&at("A"), Aggregation::Exact); + assert_eq!(one, Some(CellValue::Number(2.0))); + assert_eq!(pushed.remote.asked().lookups - spent.lookups, 1); +} + +#[test] +fn occupancy_is_one_count_whatever_the_groups_and_hidden_items() { + let wb = hidden_workbook(); + let pushed = Pushed::new(&wb.model, none_of(&wb)); + let reading = Reading::of_view(wb.active_view(), &none_of(&wb)); + let before = pushed.remote.asked(); + let coastal = CellKey::new(vec![ + ("Region".into(), "Coastal".into()), + ("Product".into(), "A".into()), + ]); + let seen = pushed.pd.occupancy(&coastal, &reading); + let spent = pushed.remote.asked(); + assert_eq!(seen, 1, "East is hidden, West holds A"); + assert_eq!(spent.counts - before.counts, 1); + assert_eq!(spent.rows_sent, before.rows_sent); +} + +#[test] +fn formula_cells_are_computed_from_the_distinct_stems_not_from_every_cell() { + let model = hand_model(); + let cells = model.data.iter_cells().count() as u64; + let pushed = Pushed::new(&model, vec!["Region".to_string()]); + let asked = pushed.remote.asked(); + assert_eq!(asked.stem_lists, 1, "one list for the one formula"); + assert_eq!( + asked.rows_sent, 2, + "Product A and Product B, of {cells} cells" + ); + assert_eq!((asked.key_lists, asked.cell_lists), (0, 0)); + assert!( + asked.summaries > 0, + "and the folds the formula reads are queries" + ); +} + +#[test] +fn a_stored_cell_is_current_and_a_formula_cell_waits_for_a_refresh() { + let mut pushed = Pushed::new(&hand_model(), vec!["Region".to_string()]); + let revenue = key("East", "A", "Revenue"); + let profit = CellKey::new(vec![ + ("Product".into(), "A".into()), + ("_Measure".into(), "Profit".into()), + ]); + let fold = |p: &Pushed| p.pd.value(&profit, Aggregation::OverHidden); + assert_eq!(fold(&pushed), Some(CellValue::Number(9.0))); + pushed + .remote + .write(revenue.clone(), Some(CellValue::Number(50.0))); + let now = pushed.pd.value(&revenue, Aggregation::Exact); + assert_eq!(now, Some(CellValue::Number(50.0)), "a read is a query"); + assert_eq!( + fold(&pushed), + Some(CellValue::Number(9.0)), + "the cache is stale" + ); + pushed.pd.refresh(); + assert_eq!(fold(&pushed), Some(CellValue::Number(49.0))); +} + +#[test] +fn a_source_that_forgets_the_hidden_items_is_caught_as_a_divergence() { + let wb = hidden_workbook(); + let views: Vec = wb.views.values().cloned().collect(); + let none = none_of(&wb); + let faulty = Pushed::through(&wb.model, none.clone(), IgnoresHidden); + let ran = differential_in(&wb.model, &views, &none, faulty, &[]); + let divergence = ran.expect_err("a wrong hidden filter must not pass"); + assert!( + matches!(*divergence.probe, Probe::Occupancy(..)), + "{divergence}" + ); + let sound = Pushed::new(&wb.model, none.clone()); + assert!(differential_in(&wb.model, &views, &none, sound, &[]).is_ok()); +} diff --git a/crates/improvise-incremental/tests/random_pushdown.rs b/crates/improvise-incremental/tests/random_pushdown.rs new file mode 100644 index 00000000..a9245f9d --- /dev/null +++ b/crates/improvise-incremental/tests/random_pushdown.rs @@ -0,0 +1,22 @@ +//! `Pushdown` over a remote table against `Eager` over random formula and cell edits (NaN, +//! `-0.0`, cycles and text among them). Evidence: `context/test-evidence/improvise-incremental.md`. + +mod support; + +use improvise_incremental::differential_in; +use proptest::prelude::*; +use support::remote::Pushed; +use support::{any_formula_edit, formula_workbook}; + +proptest! { + #[test] + fn a_pushdown_matches_eager_over_random_formula_and_cell_edits( + edits in prop::collection::vec(any_formula_edit(), 0..3), + ) { + let wb = formula_workbook(); + let none = vec!["Region".to_string()]; + let candidate = Pushed::new(&wb.model, none.clone()); + let ran = differential_in(&wb.model, &[], &none, candidate, &edits); + prop_assert!(ran.is_ok(), "{}", ran.unwrap_err()); + } +} diff --git a/crates/improvise-incremental/tests/support/live.rs b/crates/improvise-incremental/tests/support/live.rs index 8d6d2259..d88ebf60 100644 --- a/crates/improvise-incremental/tests/support/live.rs +++ b/crates/improvise-incremental/tests/support/live.rs @@ -88,6 +88,14 @@ impl CellSource for LiveTable { } Box::new(cells.into_iter()) } + + fn count(&self, select: &Select, excluding: &[(String, String)]) -> usize { + self.state.lock().unwrap().visible.count(select, excluding) + } + + fn distinct_without(&self, without: &[String]) -> Vec { + self.state.lock().unwrap().visible.distinct_without(without) + } } impl ChangeSource for LiveTable { diff --git a/crates/improvise-incremental/tests/support/mod.rs b/crates/improvise-incremental/tests/support/mod.rs index db732221..7a396c42 100644 --- a/crates/improvise-incremental/tests/support/mod.rs +++ b/crates/improvise-incremental/tests/support/mod.rs @@ -2,6 +2,7 @@ #![allow(dead_code)] pub mod live; +pub mod remote; use improvise_core::formula::ast::AggFunc; use improvise_core::formula::parse_formula; diff --git a/crates/improvise-incremental/tests/support/remote.rs b/crates/improvise-incremental/tests/support/remote.rs new file mode 100644 index 00000000..adce47c5 --- /dev/null +++ b/crates/improvise-incremental/tests/support/remote.rs @@ -0,0 +1,226 @@ +//! A remote table: an in-memory stand-in for a query engine, read by scanning and counted. +//! It is not `Eager` and shares no evaluator with it; it answers queries over rows. +#![allow(dead_code)] + +use improvise_core::model::cell::{CellKey, CellValue}; +use improvise_core::model::source::{CellSource, Select, Summary, TableSource}; +use improvise_core::model::{Aggregation, Model}; +use improvise_incremental::{Derived, Edit, Maintained, Pushdown, Reading}; +use std::sync::{Arc, Mutex}; + +/// What the engine was asked, and how many rows it sent back. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct Asked { + pub lookups: u64, + pub summaries: u64, + pub counts: u64, + pub stem_lists: u64, + pub key_lists: u64, + pub cell_lists: u64, + /// Keys and cells sent back by `keys` and `cells`: rows, not aggregates. + pub rows_sent: u64, +} + +#[derive(Default)] +pub struct Remote { + table: Mutex, + asked: Mutex, +} + +impl Remote { + pub fn of(rows: impl IntoIterator) -> Self { + let table = TableSource { + rows: rows.into_iter().collect(), + }; + Remote { + table: Mutex::new(table), + asked: Mutex::default(), + } + } + + /// Writes as another client of the database would: in place, or at the end, or removed. + pub fn write(&self, key: CellKey, value: Option) { + let mut table = self.table.lock().unwrap(); + let at = table.rows.iter().position(|(k, _)| *k == key); + match (at, value) { + (Some(i), Some(v)) => table.rows[i].1 = v, + (None, Some(v)) => table.rows.push((key, v)), + (Some(i), None) => { + table.rows.remove(i); + } + (None, None) => {} + } + } + + pub fn asked(&self) -> Asked { + *self.asked.lock().unwrap() + } + + fn ask(&self, f: impl FnOnce(&mut Asked)) { + f(&mut self.asked.lock().unwrap()); + } + + fn table(&self) -> std::sync::MutexGuard<'_, TableSource> { + self.table.lock().unwrap() + } +} + +impl CellSource for Remote { + fn value_at(&self, key: &CellKey) -> Option { + self.ask(|a| a.lookups += 1); + self.table().value_at(key) + } + + fn summarize(&self, select: &Select) -> Summary { + self.ask(|a| a.summaries += 1); + self.table().summarize(select) + } + + fn keys<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + let keys: Vec = self.table().keys(partial).collect(); + self.ask(|a| { + a.key_lists += 1; + a.rows_sent += keys.len() as u64; + }); + Box::new(keys.into_iter()) + } + + fn cells<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + let cells: Vec<_> = self.table().cells(partial).collect(); + self.ask(|a| { + a.cell_lists += 1; + a.rows_sent += cells.len() as u64; + }); + Box::new(cells.into_iter()) + } + + fn count(&self, select: &Select, excluding: &[(String, String)]) -> usize { + self.ask(|a| a.counts += 1); + self.table().count(select, excluding) + } + + fn distinct_without(&self, without: &[String]) -> Vec { + let stems = self.table().distinct_without(without); + self.ask(|a| { + a.stem_lists += 1; + a.rows_sent += stems.len() as u64; + }); + stems + } +} + +/// A source with a fault, for checking that a differential can see it: it forgets the hidden +/// items a count leaves out. +pub struct IgnoresHidden(pub Arc); + +impl CellSource for IgnoresHidden { + fn value_at(&self, key: &CellKey) -> Option { + self.0.value_at(key) + } + + fn summarize(&self, select: &Select) -> Summary { + self.0.summarize(select) + } + + fn keys<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + self.0.keys(partial) + } + + fn cells<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + self.0.cells(partial) + } + + fn count(&self, select: &Select, _excluding: &[(String, String)]) -> usize { + self.0.count(select, &[]) + } + + fn distinct_without(&self, without: &[String]) -> Vec { + self.0.distinct_without(without) + } +} + +/// A `Pushdown` over a remote table that holds the model's cells; the model keeps no store. +/// A cell edit is a write to the table, as another client's would be, then a refresh. +pub struct Pushed { + pub pd: Pushdown, + pub remote: Arc, +} + +impl Pushed { + pub fn new(model: &Model, none_cats: Vec) -> Pushed { + let remote = Arc::new(Remote::of(rows_of(model))); + let pd = Pushdown::new(bare(model), remote.clone(), none_cats); + Pushed { pd, remote } + } + + /// The same, reading the table through `wrap`. + pub fn through( + model: &Model, + none_cats: Vec, + wrap: impl FnOnce(Arc) -> Q, + ) -> Pushed { + let remote = Arc::new(Remote::of(rows_of(model))); + let pd = Pushdown::new(bare(model), Arc::new(wrap(remote.clone())), none_cats); + Pushed { pd, remote } + } +} + +fn rows_of(model: &Model) -> Vec<(CellKey, CellValue)> { + model + .data + .iter_cells() + .map(|(k, v)| (k, v.clone())) + .collect() +} + +/// `model` with no cells of its own. +fn bare(model: &Model) -> Model { + let mut bare = model.clone(); + bare.data = improvise_core::model::cell::DataStore::new(); + bare +} + +impl Derived for Pushed { + fn value(&self, key: &CellKey, agg: Aggregation) -> Option { + self.pd.value(key, agg) + } + + fn raw_fold(&self, partial: &CellKey) -> Option { + self.pd.raw_fold(partial) + } + + fn covered(&self, key: &CellKey, reading: &Reading) -> Option { + self.pd.covered(key, reading) + } + + fn total(&self, key: &CellKey, over: &[String], reading: &Reading) -> Option { + self.pd.total(key, over, reading) + } + + fn occupancy(&self, partial: &CellKey, reading: &Reading) -> usize { + self.pd.occupancy(partial, reading) + } +} + +impl Maintained for Pushed { + fn apply(&mut self, edit: &Edit) { + match edit { + Edit::SetCell(key, value) => self.remote.write(key.clone(), Some(value.clone())), + Edit::ClearCell(key) => self.remote.write(key.clone(), None), + _ => return self.pd.define(edit), + } + self.pd.refresh(); + } +} From 4a49398f1aa9413abcab0efbc1e65989add550bf Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:52:58 -0700 Subject: [PATCH 07/14] 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 bebaeb2d61a1009dbe5e85e598558c6608a3f569 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:57:13 -0700 Subject: [PATCH 08/14] fix(core): a model over a source refuses every write in every build and drops its own cells (improvise-rnak) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/source.md | 11 +++-- context/test-evidence/core.md | 25 +++++++++++ crates/improvise-core/src/model/over_tests.rs | 42 +++++++++++++++++++ crates/improvise-core/src/model/types.rs | 16 +++++-- crates/improvise-core/src/view/layout.rs | 26 ++++++++++++ 5 files changed, 112 insertions(+), 8 deletions(-) diff --git a/context/code/source.md b/context/code/source.md index f83c467f..a04c1706 100644 --- a/context/code/source.md +++ b/context/code/source.md @@ -65,10 +65,13 @@ the owned `DataStore`. That is the whole evaluator hook (improvise-rnak, bw4p.14 there is still one evaluator, `Model`'s, and it computes every formula value, over a source whose cells it never owns. -1. **A model over a source is read-only.** `set_cell` and `clear_cell` `debug_assert` that no source - is set, because they write the owned store, which is then not read. A release build would drop the - write silently; the assert is the guard, not the design. The design is that a caller who holds a - source writes to the source (ADR 003: the trait has no writes), and the model is built over it. +1. **A model over a source is read-only, in every build.** `set_cell`, `clear_cell`, `remove_category` and + `remove_item` `assert!` that no source is set (a `debug_assert` first shipped and was lost in release: + the write went to the owned store, which nothing reads). A caller who holds a source writes to the + source (ADR 003: the trait has no writes). `Model::over` drops the model's own cells, so no stale copy + is left for `get_cell`, persistence or the harness to read. `data` is `pub`, so a caller can still write + it directly; it is not read, and nothing prevents that. Structural edits (a category or item removed) + are refused too, because the source's cells would still carry them. 2. **A shared source changes behind its readers by interior mutability of its own.** The model holds an `Arc`, so a database connection or a test double behind a lock is the same object the backend feeds from. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 7698f16c..32e16dc9 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -2397,3 +2397,28 @@ Extremes with no numbers, and a NaN as an extreme, are dropped by `Numbers::from 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. + +### a_model_over_a_source_refuses_a_clear + +Review of #357: the `clear_cell` guard had no test. The refusal is an `assert!`, so the test holds in a +release build too (`cargo test --release -p improvise-core --lib over_tests` was run: the earlier +`debug_assert!` failed all four refusal tests there, `a_model_over_a_source_refuses_a_write` among +them, and was the bug). Rival: the guard removed from `clear_cell` fails this test only. + +### a_model_over_a_source_refuses_to_remove_a_category + +`remove_category` swept the owned store with no guard. Rival: the guard removed. + +### a_model_over_a_source_refuses_to_remove_an_item + +The same for `remove_item`. + +### putting_a_populated_model_over_a_source_drops_its_own_cells + +`Model::over` empties the owned store, so `get_cell` on an over-model is `None` for a cell the +source holds, and the evaluator still reads the source's value. + +### records_mode_reads_the_records_from_a_source + +Review of #357: a records-mode `GridLayout` over a source. `group_records` reading the owned store +left all 563 tests green; with this test it fails (the grid has no rows). diff --git a/crates/improvise-core/src/model/over_tests.rs b/crates/improvise-core/src/model/over_tests.rs index d8078a5d..305e8822 100644 --- a/crates/improvise-core/src/model/over_tests.rs +++ b/crates/improvise-core/src/model/over_tests.rs @@ -219,6 +219,48 @@ fn a_model_over_a_source_refuses_a_write() { over.set_cell(key("East", "A", "Revenue"), CellValue::Number(1.0)); } +#[test] +#[should_panic(expected = "read-only")] +fn a_model_over_a_source_refuses_a_clear() { + let mut over = lent(&fixture()); + over.clear_cell(&key("East", "A", "Revenue")); +} + +#[test] +#[should_panic(expected = "read-only")] +fn a_model_over_a_source_refuses_to_remove_a_category() { + let mut over = lent(&fixture()); + over.remove_category("Product"); +} + +#[test] +#[should_panic(expected = "read-only")] +fn a_model_over_a_source_refuses_to_remove_an_item() { + let mut over = lent(&fixture()); + over.remove_item("Product", "A"); +} + +#[test] +fn putting_a_populated_model_over_a_source_drops_its_own_cells() { + let owned = fixture(); + assert!(owned.data.iter_cells().count() > 0); + let rows = owned + .data + .iter_cells() + .map(|(k, v)| (k, v.clone())) + .collect(); + let over = owned.clone().over(Arc::new(TableSource { rows })); + assert_eq!(over.data.iter_cells().count(), 0, "no stale second copy"); + let cell = key("East", "A", "Revenue"); + assert_eq!( + over.get_cell(&cell), + None, + "the owned store is empty, the source is not" + ); + let read = over.evaluate_aggregated(&cell, Aggregation::Exact); + assert_eq!(read, owned.evaluate_aggregated(&cell, Aggregation::Exact)); +} + #[test] fn the_records_a_drill_freezes_are_read_from_the_source() { use crate::view::layout::drill_snapshot; diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index d9c161e8..0e5c0193 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -430,13 +430,19 @@ impl Model { } } - /// This model, reading its raw cells from `source` and not `data`: read-only. - /// Why: `context/code/source.md`. + /// This model, reading its raw cells from `source` and not `data`: read-only. Its own + /// cells are dropped, so no stale second copy is left to read. Why: `context/code/source.md`. pub fn over(mut self, source: SharedSource) -> Self { + self.data = DataStore::new(); self.external = Some(source); self } + /// Refuses, in every build, a write the source would never see. + fn writable(&self) { + assert!(self.external.is_none(), "{READ_ONLY}"); + } + /// Add a pivot category, enforcing `MAX_CATEGORIES` over regular ones. /// The caller registers it on every view — normally `Workbook`. pub fn add_category(&mut self, name: impl Into) -> Result { @@ -480,6 +486,7 @@ impl Model { /// Remove a category, its cells and the formulas targeting it. The caller /// removes it from the views — normally `Workbook`. pub fn remove_category(&mut self, name: &str) { + self.writable(); if !self.categories.contains_key(name) || CategoryKind::of_name(name).is_virtual() { return; } @@ -502,6 +509,7 @@ impl Model { /// An item, the formulas targeting it and the cells naming it. The sweep /// is `remove_category`'s one level down — `context/code/core.md` §9. pub fn remove_item(&mut self, cat_name: &str, item_name: &str) { + self.writable(); if let Some(cat) = self.categories.get_mut(cat_name) { cat.remove_item(item_name); } @@ -552,12 +560,12 @@ impl Model { } pub fn set_cell(&mut self, key: CellKey, value: CellValue) { - debug_assert!(self.external.is_none(), "{READ_ONLY}"); + self.writable(); self.data.set(key, value); } pub fn clear_cell(&mut self, key: &CellKey) { - debug_assert!(self.external.is_none(), "{READ_ONLY}"); + self.writable(); self.data.remove(key); } diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 5c8ae905..4a4520d9 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -1348,6 +1348,32 @@ mod tests { assert_eq!(layout.col_label(0), "X"); } + /// The records a records-mode grid draws come from the source the model is over. + #[test] + fn records_mode_reads_the_records_from_a_source() { + use crate::model::source::TableSource; + let mut wb = records_workbook(); + let v = wb.active_view_mut(); + v.set_axis("_Index", Axis::Row); + v.set_axis("_Dim", Axis::Column); + let rows = wb.model.data.iter_cells().map(|(k, v)| (k, v.clone())); + let over = wb.model.clone().over(std::sync::Arc::new(TableSource { + rows: rows.collect(), + })); + let (owned, lent) = ( + GridLayout::new(&wb.model, wb.active_view()), + GridLayout::new(&over, wb.active_view()), + ); + assert!(lent.is_records_mode()); + assert_eq!(lent.row_count(), 1, "the source holds the records"); + for col in 0..owned.col_count() { + assert_eq!(lent.records_display(0, col), owned.records_display(0, col)); + } + let revenue = (0..lent.col_count()).find(|&c| lent.col_label(c) == "Revenue"); + let revenue = revenue.unwrap(); + assert_eq!(lent.records_display(0, revenue).as_deref(), Some("100")); + } + #[test] fn records_mode_activated_when_index_and_dim_on_axes() { let mut wb = records_workbook(); From 82fbe16ea4761240491e71e69c54bf4f4a441cdb Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 19:00:51 -0700 Subject: [PATCH 09/14] fix(incremental): over takes the exported feed alias, drops the model's cells and refuses cell edits in every build (improvise-rnak) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/improvise-incremental.md | 10 ++- context/code/source.md | 5 +- .../test-evidence/improvise-incremental.md | 25 ++++++ crates/improvise-core/src/model/source.rs | 4 +- .../improvise-incremental/src/incremental.rs | 17 ++-- .../tests/live_source.rs | 79 ++++++++++++++++++- 6 files changed, 122 insertions(+), 18 deletions(-) diff --git a/context/code/improvise-incremental.md b/context/code/improvise-incremental.md index 0791e1de..ac5d592b 100644 --- a/context/code/improvise-incremental.md +++ b/context/code/improvise-incremental.md @@ -407,13 +407,17 @@ from the source's folds, never pushed down; `Derived` stays in this crate. what a database does inside a read transaction and what `DataStore` does already (its log is drained in the step that made the edit). `&self` because a live source is shared with the model that reads it and changes behind it by interior mutability of its own. -2. **`Incremental::over(model, feed, none_cats)`**: the model is put over the feed (`Model::over`) and - keeps no cells of its own. Its categories, formulas and aggregations are the definition; the +2. **`Incremental::over(model, feed, none_cats)`** takes a `SharedFeed`, the alias core exports (a generic + `Arc` could not take it: `S` is `Sized`). The model is put over the feed (`Model::over`), which + drops the cells it came with, so none is left for `get_cell`, persistence or the harness to read. Its categories, formulas and aggregations are the definition; the source holds the data. Shapes and stems are built from the feed as it stands. 3. **`sync()` drains and maintains; `apply_changes(&[CellChange])` maintains changes the source already reflects**, for a source whose changes reach the backend by another route (a trigger table, a channel). `Maintained::apply` on a fed backend takes definition edits (a formula, an aggregation) - and drains: a cell edit is the source's to make, and `Model::set_cell` refuses it in a debug build. + and drains: a cell edit is the source's to make, and `apply` refuses it with an `assert!` in every + build (a release build lost it silently when only `Model::set_cell`'s `debug_assert` stood in the way). + **A feed has one drainer**: a drain is destructive, so two backends over one feed split its changes + (`context/code/source.md`, `ChangeSource`). 4. **Freshness is the source's.** ADR 003 left "polled, triggered, or stale until refreshed" open: `sync` is the poll, and when it is called is the host's decision. Nothing here schedules it. 5. **What a change costs** is what it costs for the owned store: two group updates and two exact-sum diff --git a/context/code/source.md b/context/code/source.md index 056046c4..f30d8d83 100644 --- a/context/code/source.md +++ b/context/code/source.md @@ -98,7 +98,10 @@ The contract that matters is that a source's cells are those as of the last drai now plus the changes drained later counts no change twice or not at all. Why, and how `Incremental` uses it: `context/code/improvise-incremental.md`, step 7. `DataStore` is not a `ChangeSource`: its log is `&mut` and is drained by the owner in the step that made the edit. A `SharedFeed` is the `Arc` a -backend and a model share. +backend and a model share. **A feed has one drainer**: a drain empties the pending list, so two +backends over one feed split the changes between them (measured in review: the same write gave one +backend a Revenue of 128 and the other 38, and the second then disagreed with its own exact reads). +`a_feed_has_one_drainer` pins the behaviour. Kb-style invariant: `invariant(change_source, one_drainer)`. ## Building a `Summary` from an engine's answer diff --git a/context/test-evidence/improvise-incremental.md b/context/test-evidence/improvise-incremental.md index 61011e72..943bd9a3 100644 --- a/context/test-evidence/improvise-incremental.md +++ b/context/test-evidence/improvise-incremental.md @@ -842,3 +842,28 @@ the model not over the feed fails it with the others. `Fed` against `Eager` over random formula and cell edits on the formula fixture (no views, so the binary stays under two seconds). Rivals: `sync` drains nothing, the model not over the feed, a shape built from an empty store and the stems from the owned store all fail it. + +### a_backend_can_be_built_over_the_exported_feed_alias + +Review of #358: `over(Arc)` could not take the `SharedFeed` the same PR exports. The test builds +a backend from a `SharedFeed`; it did not compile against the generic signature. + +### a_populated_model_put_over_a_feed_keeps_no_cells_of_its_own + +Review of #358: `Fed::new` emptied the store before the call, so no test passed a populated model. This +one does, asserts the owned store is empty afterwards and that every harness probe agrees with +`Eager`. Rival: `Model::over` keeping the cells fails it, and only it. + +### a_cell_edit_on_a_fed_backend_is_refused + +`Maintained::apply` with a `SetCell` panics with the backend's own message in every build. Rival: the +assert removed (the test then sees the model's message, not the backend's, and fails). + +### a_cell_clear_on_a_fed_backend_is_refused + +The same for `ClearCell`. + +### a_feed_has_one_drainer + +Review of #358: two backends over one feed; one write; `a.sync(); b.sync()`. The first sees it, the +second does not. This pins the behaviour as documented rather than as a guarantee: a drain is destructive. diff --git a/crates/improvise-core/src/model/source.rs b/crates/improvise-core/src/model/source.rs index 944ae13f..2cd70a26 100644 --- a/crates/improvise-core/src/model/source.rs +++ b/crates/improvise-core/src/model/source.rs @@ -192,8 +192,8 @@ impl Summary { /// its readers (a database) does so through interior mutability of its own. pub type SharedSource = std::sync::Arc; -/// A source that reports its changes. Its cells are the state after the changes it has drained, -/// so a snapshot read now plus the changes drained later counts no change twice or not at all. +/// A source that reports its changes; its cells are the state as of its last drain. +/// A drain is destructive: a feed has one drainer (`context/code/source.md`). pub trait ChangeSource: CellSource { /// The changes since the last drain, oldest first; reads see them from now on. fn drain_changes(&self) -> Vec; diff --git a/crates/improvise-incremental/src/incremental.rs b/crates/improvise-incremental/src/incremental.rs index 52b4bdff..c83d9746 100644 --- a/crates/improvise-incremental/src/incremental.rs +++ b/crates/improvise-incremental/src/incremental.rs @@ -9,11 +9,10 @@ use crate::{Derived, Edit, Maintained, Reading, cell_delta}; use improvise_core::formula::ast::AggFunc; use improvise_core::model::category::MEASURE_CATEGORY; use improvise_core::model::cell::{CellChange, CellKey, CellValue}; -use improvise_core::model::source::{ChangeSource, SharedFeed}; +use improvise_core::model::source::{SharedFeed, SharedSource}; use improvise_core::model::types::{Covers, settled}; use improvise_core::model::{Aggregation, Model}; use std::collections::BTreeMap; -use std::sync::Arc; /// How a stem's formulas are evaluated: in dependency order where one pass is exact, or by /// eager's passes always. The two must agree; `Plain` is the reference. @@ -47,12 +46,9 @@ impl Incremental { /// A backend over `feed`, which holds the cells and reports their changes; `model`'s own /// store is not read. Reasoning: `context/code/improvise-incremental.md`, step 7. - pub fn over( - model: Model, - feed: Arc, - none_cats: Vec, - ) -> Self { - let model = model.over(feed.clone()); + pub fn over(model: Model, feed: SharedFeed, none_cats: Vec) -> Self { + let source: SharedSource = feed.clone(); + let model = model.over(source); Self::start(model, none_cats, Some(feed)) } @@ -390,6 +386,11 @@ impl Derived for Incremental { impl Maintained for Incremental { fn apply(&mut self, edit: &Edit) { + let refused = self.feed.is_some() && edit.key().is_some(); + assert!( + !refused, + "a fed backend is read-only: cell edits go through the source" + ); edit.apply(&mut self.model); let changes = self.drain(); self.absorb(Some(edit), &changes); diff --git a/crates/improvise-incremental/tests/live_source.rs b/crates/improvise-incremental/tests/live_source.rs index ba6a622a..493c3741 100644 --- a/crates/improvise-incremental/tests/live_source.rs +++ b/crates/improvise-incremental/tests/live_source.rs @@ -5,12 +5,14 @@ mod support; use improvise_core::formula::ast::AggFunc; use improvise_core::model::cell::{CellKey, CellValue}; -use improvise_core::model::source::ChangeSource; +use improvise_core::model::source::{ChangeSource, SharedFeed}; use improvise_core::model::{Aggregation, Model}; use improvise_incremental::{ - Derived, Eager, Edit, Maintained, Meter, differential_in, edit_probes, probes, same_value, + Derived, Eager, Edit, Incremental, Maintained, Meter, differential_in, edit_probes, probes, + same_value, }; -use support::live::Fed; +use std::sync::Arc; +use support::live::{Fed, LiveTable}; use support::{corpus, edits_for, hand_model, key}; #[test] @@ -108,7 +110,7 @@ fn row(r: usize, p: usize, m: usize) -> CellKey { ]) } -/// `n` regions by `n` products by 6 months of one measure, as a model with a profit-style formula. +/// `n` regions by `n` products by 6 months of one measure, with no formula. fn table(n: usize) -> Model { let mut model = Model::new("table"); for (cat, prefix, count) in [("Region", "r", n), ("Product", "p", n), ("Month", "m", 6)] { @@ -211,3 +213,72 @@ fn an_overwrite_by_the_same_value_moves_nothing() { assert_eq!(fed.inc.raw_fold(&revenue), before); assert_eq!(before, Some(CellValue::Number(23.0))); } + +#[test] +fn a_backend_can_be_built_over_the_exported_feed_alias() { + let model = hand_model(); + let rows = model.data.iter_cells().map(|(k, v)| (k, v.clone())); + let feed: SharedFeed = Arc::new(LiveTable::of(rows)); + let mut inc = Incremental::over(model.clone(), feed, vec![]); + let revenue = CellKey::new(vec![("_Measure".into(), "Revenue".into())]); + assert_eq!(inc.raw_fold(&revenue), Some(CellValue::Number(23.0))); + inc.sync(); +} + +#[test] +fn a_populated_model_put_over_a_feed_keeps_no_cells_of_its_own() { + let (model, none) = (hand_model(), vec!["Region".to_string()]); + assert!( + model.data.iter_cells().count() > 0, + "the input is populated" + ); + let rows = model.data.iter_cells().map(|(k, v)| (k, v.clone())); + let live = Arc::new(LiveTable::of(rows)); + let inc = Incremental::over(model.clone(), live, none.clone()); + assert_eq!(inc.model().data.iter_cells().count(), 0, "no stale copy"); + assert_eq!(inc.model().get_cell(&key("East", "A", "Revenue")), None); + let eager = Eager::new(model, none); + for probe in probes(eager.model()) { + let (want, got) = (probe.ask(&eager), probe.ask(&inc)); + assert!( + same_value(&want, &got), + "{probe:?}: eager {want:?} != fed {got:?}" + ); + } +} + +#[test] +#[should_panic(expected = "cell edits go through the source")] +fn a_cell_edit_on_a_fed_backend_is_refused() { + let mut fed = Fed::new(&hand_model(), vec![]); + let edit = Edit::SetCell(key("East", "A", "Revenue"), CellValue::Number(1.0)); + fed.inc.apply(&edit); +} + +#[test] +#[should_panic(expected = "cell edits go through the source")] +fn a_cell_clear_on_a_fed_backend_is_refused() { + let mut fed = Fed::new(&hand_model(), vec![]); + fed.inc.apply(&Edit::ClearCell(key("East", "A", "Revenue"))); +} + +#[test] +fn a_feed_has_one_drainer() { + let model = hand_model(); + let rows = model.data.iter_cells().map(|(k, v)| (k, v.clone())); + let live = Arc::new(LiveTable::of(rows)); + let mut a = Incremental::over(model.clone(), live.clone(), vec![]); + let mut b = Incremental::over(model, live.clone(), vec![]); + let revenue = CellKey::new(vec![("_Measure".into(), "Revenue".into())]); + let (before_a, before_b) = (a.raw_fold(&revenue), b.raw_fold(&revenue)); + assert_eq!(before_a, before_b); + live.write(key("East", "A", "Revenue"), Some(CellValue::Number(100.0))); + a.sync(); + b.sync(); + let (after_a, after_b) = (a.raw_fold(&revenue), b.raw_fold(&revenue)); + assert_ne!(after_a, before_a, "the first drainer saw the write"); + assert_eq!( + after_b, before_b, + "the second saw nothing: a drain is destructive" + ); +} From 53868bb4d6d1534c938ec96516f559f89cdab3ff Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 19:06:11 -0700 Subject: [PATCH 10/14] fix(incremental): pushdown takes the source alias, refuses cell edits, shares Eager's reads and pins stem order and refresh snapshots (improvise-rnak) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/improvise-incremental.md | 25 ++- .../test-evidence/improvise-incremental.md | 33 ++++ crates/improvise-incremental/src/eager.rs | 18 +- crates/improvise-incremental/src/lib.rs | 1 + crates/improvise-incremental/src/pushdown.rs | 34 ++-- crates/improvise-incremental/src/reads.rs | 33 ++++ .../improvise-incremental/tests/pushdown.rs | 160 +++++++++++++++++- .../tests/support/remote.rs | 91 +++++++++- 8 files changed, 359 insertions(+), 36 deletions(-) create mode 100644 crates/improvise-incremental/src/reads.rs diff --git a/context/code/improvise-incremental.md b/context/code/improvise-incremental.md index b2e57a1f..756cfda0 100644 --- a/context/code/improvise-incremental.md +++ b/context/code/improvise-incremental.md @@ -458,7 +458,30 @@ change log, no stems. These are the readings taken. same thing. 5. **`define(edit)` takes a formula or an aggregation edit and refreshes.** A cell edit is the source's: `Model::set_cell` refuses it in a debug build. -6. **What is and is not proven.** The tests run against `Remote` (`tests/support/remote.rs`), a scanning +6. **What the differential proves, and what it does not.** `value`, `raw_fold`, `covered` and `total` are + the same four functions for `Eager` and `Pushdown` (`reads.rs`; they were copies, and are shared now), + so a Pushdown-against-Eager differential cannot see a mistake in them. `Pushdown::members` repeats + `Eager::group_holds` in a different form (a span list against a predicate), and that repetition + is compared. What the differential proves is the **source's** semantics: that `summarize`, `count` + and `distinct_without` give, for every select the evaluator and the views make, the answers a + `DataStore` gives. The evaluator over a source is proven equal to the evaluator over the owned + store in `improvise-core` (`over_tests`). Nothing else is claimed. +7. **A refresh wants one snapshot.** `refresh` asks `distinct_without` and then folds, so a source that + changes between the queries can serve them from different revisions. The passes heal what they + can: a write between two folds is read again by the next pass, and the cache ends equal to the new + state's values (`writes_between_two_queries_of_a_refresh_are_healed_by_the_fixed_point`). A stem + that appears after the stem list was taken has no cell until the next refresh + (`a_stem_that_appears_between_the_stem_list_and_the_folds_waits_for_the_next_refresh`). A real source + should serve a refresh from one read transaction; nothing here can ask it to. +8. **The order of the stems is part of the contract.** The passes of the fixed point visit stems in the + order `distinct_without` lists them, and a document whose stems read each other settles in as many + passes as its dependency chain runs against that order, with a bound of sixteen. A chain of + eighteen stems settles in one pass listed along the chain and not at all listed against it + (`the_order_stems_are_listed_in_decides_a_result_and_a_source_must_keep_it`). No fixture before it + had a result that depended on the order, and a source listing stems sorted passed everything. + The first-seen order is `DataStore`'s insertion order; a source with no order of its own must + fix one. +9. **What is and is not proven.** The tests run against `Remote` (`tests/support/remote.rs`), a scanning in-memory table with counters, that shares no evaluator with `Eager` and answers queries over rows. Without a real SQL engine only the **contract** is proven: that a source answering these six queries as specified gives `Eager`'s answers, and what each read costs in queries. Nothing here diff --git a/context/test-evidence/improvise-incremental.md b/context/test-evidence/improvise-incremental.md index 57500731..2cab469e 100644 --- a/context/test-evidence/improvise-incremental.md +++ b/context/test-evidence/improvise-incremental.md @@ -924,3 +924,36 @@ The same for `ClearCell`. Review of #358: two backends over one feed; one write; `a.sync(); b.sync()`. The first sees it, the second does not. This pins the behaviour as documented rather than as a guarantee: a drain is destructive. + +### the_order_stems_are_listed_in_decides_a_result_and_a_source_must_keep_it + +Review of #360: stem order was invisible to every differential (a sorted `distinct_without` went +uncaught). Eighteen products are stored in chain order with names that sort the other way, and one nested +`IF` formula reads the product before it. Along the chain it settles in one pass (the last link is 18); +sorted it needs one pass per link and the bound of sixteen leaves it `circular`. The test asserts both +values, that a sound source passes the differential and that a source listing the stems sorted is a +divergence. Rival: `DataStore::distinct_without` sorting fails it (the oracle then gives `circular`, so +the first assertion fails). Before it, a source sorting its stems passed every differential; only the core stems test (which compares two stores) saw a sort. + +### writes_between_two_queries_of_a_refresh_are_healed_by_the_fixed_point + +Review of #360: a source that changes between the queries of one refresh. Another client's writes land +right after the first fold; the cached Profit of Product A ends at 53, the new state's value, and not at +13 (the old revenue less the new cost). The documented guard is one snapshot per refresh. Rival: +none; this pins a behaviour that the fixed point gives and the note says may not be relied on. + +### a_stem_that_appears_between_the_stem_list_and_the_folds_waits_for_the_next_refresh + +The stem list is taken, then Product C is written: it has no formula cell until the next refresh, +which has it (5). + +### a_cell_edit_on_a_pushdown_is_refused + +Review of #360: `define` applied a cell edit to the model, whose guard was a `debug_assert`; now an +`assert!` of its own, with its own message. Rival: the assert removed fails it (the model's message +differs). + +### a_pushdown_can_be_built_over_the_exported_source_alias + +`Pushdown::new` took a generic `Arc` that could not take a `SharedSource`; the test builds one from +the alias and did not compile against the generic signature. diff --git a/crates/improvise-incremental/src/eager.rs b/crates/improvise-incremental/src/eager.rs index 7bb674eb..2edd2a1d 100644 --- a/crates/improvise-incremental/src/eager.rs +++ b/crates/improvise-incremental/src/eager.rs @@ -1,6 +1,6 @@ //! Today's evaluator behind the trait. Temporary (ADR 009 clause 2). -use crate::{Derived, Edit, Maintained, Reading}; +use crate::{Derived, Edit, Maintained, Reading, reads}; use improvise_core::model::cell::{CellKey, CellValue}; use improvise_core::model::{Aggregation, Model}; @@ -36,27 +36,19 @@ impl Eager { impl Derived for Eager { fn value(&self, key: &CellKey, agg: Aggregation) -> Option { - self.model.evaluate_aggregated(key, agg) + reads::value(&self.model, key, agg) } fn raw_fold(&self, partial: &CellKey) -> Option { - match self.model.formula_for(partial) { - Some(_) => None, - None => self - .model - .evaluate_aggregated(partial, Aggregation::OverHidden), - } + reads::raw_fold(&self.model, partial) } fn covered(&self, key: &CellKey, reading: &Reading) -> Option { - let covers = |cat: &str, item: &str| reading.covers(cat, item); - self.model.evaluate_covering(key, reading.agg, &covers) + reads::covered(&self.model, key, reading) } fn total(&self, key: &CellKey, over: &[String], reading: &Reading) -> Option { - let covers = |cat: &str, item: &str| reading.covers(cat, item); - self.model - .evaluate_spanning(key, over, reading.agg, &covers) + reads::total(&self.model, key, over, reading) } fn occupancy(&self, partial: &CellKey, reading: &Reading) -> usize { diff --git a/crates/improvise-incremental/src/lib.rs b/crates/improvise-incremental/src/lib.rs index ae6b9d70..e809ed5a 100644 --- a/crates/improvise-incremental/src/lib.rs +++ b/crates/improvise-incremental/src/lib.rs @@ -11,6 +11,7 @@ mod harness; mod incremental; mod pushdown; mod reading; +mod reads; mod shapes; pub use delta::{CellDelta, DeltaValue, cell_delta, moved_records}; diff --git a/crates/improvise-incremental/src/pushdown.rs b/crates/improvise-incremental/src/pushdown.rs index 90ef6f67..c1bd8099 100644 --- a/crates/improvise-incremental/src/pushdown.rs +++ b/crates/improvise-incremental/src/pushdown.rs @@ -1,11 +1,10 @@ //! Derived values over a source that answers aggregate queries and reports no changes. //! Reasoning: `context/code/improvise-incremental.md`, step 7. -use crate::{Derived, Edit, Reading}; +use crate::{Derived, Edit, Reading, reads}; use improvise_core::model::cell::{CellKey, CellValue}; -use improvise_core::model::source::{CellSource, Select}; +use improvise_core::model::source::{Select, SharedSource}; use improvise_core::model::{Aggregation, Model}; -use std::sync::Arc; /// A model whose cells are in `source`: a fold is a query, the formula cells are computed in RAM /// by the model's own evaluator from those folds, and nothing is kept per shape or per change. @@ -16,11 +15,7 @@ pub struct Pushdown { impl Pushdown { /// `definition` supplies the categories, formulas and aggregations; its own store is not read. - pub fn new( - definition: Model, - source: Arc, - none_cats: Vec, - ) -> Self { + pub fn new(definition: Model, source: SharedSource, none_cats: Vec) -> Self { let mut backend = Pushdown { model: definition.over(source), none_cats, @@ -33,14 +28,19 @@ impl Pushdown { &self.model } - /// Computes the formula cells again from the source as it now stands. A read of a stored - /// cell or a fold is a query and always current; the formula cache is the one thing held. + /// Computes the formula cells again from the source as it now stands; only they are held. + /// The source should serve a refresh from one snapshot: improvise-incremental.md, step 7. pub fn refresh(&mut self) { self.model.recompute_formulas(&self.none_cats); } /// A change to the definition: a formula or an aggregation. Cells belong to the source. pub fn define(&mut self, edit: &Edit) { + let refused = edit.key().is_some(); + assert!( + !refused, + "a pushdown is read-only: cell edits go through the source" + ); edit.apply(&mut self.model); self.refresh(); } @@ -64,25 +64,19 @@ impl Pushdown { impl Derived for Pushdown { fn value(&self, key: &CellKey, agg: Aggregation) -> Option { - self.model.evaluate_aggregated(key, agg) + reads::value(&self.model, key, agg) } fn raw_fold(&self, partial: &CellKey) -> Option { - match self.model.formula_for(partial) { - Some(_) => None, - None => self.value(partial, Aggregation::OverHidden), - } + reads::raw_fold(&self.model, partial) } fn covered(&self, key: &CellKey, reading: &Reading) -> Option { - let covers = |cat: &str, item: &str| reading.covers(cat, item); - self.model.evaluate_covering(key, reading.agg, &covers) + reads::covered(&self.model, key, reading) } fn total(&self, key: &CellKey, over: &[String], reading: &Reading) -> Option { - let covers = |cat: &str, item: &str| reading.covers(cat, item); - self.model - .evaluate_spanning(key, over, reading.agg, &covers) + reads::total(&self.model, key, over, reading) } fn occupancy(&self, partial: &CellKey, reading: &Reading) -> usize { diff --git a/crates/improvise-incremental/src/reads.rs b/crates/improvise-incremental/src/reads.rs new file mode 100644 index 00000000..e0f5a169 --- /dev/null +++ b/crates/improvise-incremental/src/reads.rs @@ -0,0 +1,33 @@ +//! The reads a model answers for a view, shared by every backend that leaves them to the model. +//! Reasoning: `context/code/improvise-incremental.md`, step 7. + +use crate::Reading; +use improvise_core::model::cell::{CellKey, CellValue}; +use improvise_core::model::{Aggregation, Model}; + +pub fn value(model: &Model, key: &CellKey, agg: Aggregation) -> Option { + model.evaluate_aggregated(key, agg) +} + +/// Never a formula: `None` for a partial naming a formula target. +pub fn raw_fold(model: &Model, partial: &CellKey) -> Option { + match model.formula_for(partial) { + Some(_) => None, + None => value(model, partial, Aggregation::OverHidden), + } +} + +pub fn covered(model: &Model, key: &CellKey, reading: &Reading) -> Option { + let covers = |cat: &str, item: &str| reading.covers(cat, item); + model.evaluate_covering(key, reading.agg, &covers) +} + +pub fn total( + model: &Model, + key: &CellKey, + over: &[String], + reading: &Reading, +) -> Option { + let covers = |cat: &str, item: &str| reading.covers(cat, item); + model.evaluate_spanning(key, over, reading.agg, &covers) +} diff --git a/crates/improvise-incremental/tests/pushdown.rs b/crates/improvise-incremental/tests/pushdown.rs index 2b4eaea0..566ee079 100644 --- a/crates/improvise-incremental/tests/pushdown.rs +++ b/crates/improvise-incremental/tests/pushdown.rs @@ -4,11 +4,18 @@ mod support; use improvise_core::formula::ast::AggFunc; +use improvise_core::formula::parse_formula; use improvise_core::model::cell::{CellKey, CellValue}; +use improvise_core::model::source::SharedSource; use improvise_core::model::{Aggregation, Model}; use improvise_core::view::View; use improvise_incremental::{Derived, Edit, Probe, Reading, differential_in}; -use support::remote::{IgnoresHidden, Pushed}; +use improvise_incremental::{Eager, Pushdown}; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +use support::remote::{ + ChangesAfterAFold, ChangesAfterStems, Hooked, IgnoresHidden, Pushed, Remote, SortsStems, +}; use support::{ corpus, edits_for, grouped_model, hand_model, hidden_counted_workbook, hidden_workbook, key, none_of, ratio_workbook, wide_workbook, @@ -195,3 +202,154 @@ fn a_source_that_forgets_the_hidden_items_is_caught_as_a_divergence() { let sound = Pushed::new(&wb.model, none.clone()); assert!(differential_in(&wb.model, &views, &none, sound, &[]).is_ok()); } + +/// Eighteen products stored in chain order and named to sort the other way, and a formula that +/// reads the product before it: one pass along the chain, one per link against it. +fn chain() -> Model { + let name = |k: usize| format!("n{}", 100 - k); + let mut model = Model::new("chain"); + model.add_category("Product").unwrap(); + model.category_mut("_Measure").unwrap().add_item("Revenue"); + for k in 1..=18 { + model.category_mut("Product").unwrap().add_item(name(k)); + let cell = CellKey::new(vec![ + ("Product".into(), name(k)), + ("_Measure".into(), "Revenue".into()), + ]); + model.set_cell(cell, CellValue::Number(k as f64)); + } + let mut text = "0".to_string(); + for k in (2..=18).rev() { + text = format!( + "IF((Product IS {}) > 0, {} + 1, {text})", + name(k), + name(k - 1) + ); + } + text = format!("IF((Product IS {}) > 0, 1, {text})", name(1)); + model.add_formula(parse_formula(&format!("Chain = {text}"), "_Measure").unwrap()); + model +} + +#[test] +fn the_order_stems_are_listed_in_decides_a_result_and_a_source_must_keep_it() { + let model = chain(); + let last = CellKey::new(vec![ + ("Product".into(), "n82".into()), + ("_Measure".into(), "Chain".into()), + ]); + let eager = Eager::new(model.clone(), vec![]); + let want = eager.value(&last, Aggregation::OverHidden); + assert_eq!( + want, + Some(CellValue::Number(18.0)), + "along the chain it settles" + ); + let sound = Pushed::new(&model, vec![]); + assert_eq!(sound.pd.value(&last, Aggregation::OverHidden), want); + let ran = differential_in(&model, &[], &[], sound, &[]); + assert!(ran.is_ok(), "{}", ran.unwrap_err()); + let sorted = Pushed::through(&model, vec![], |remote| Hooked(remote, SortsStems)); + let got = sorted.pd.value(&last, Aggregation::OverHidden); + assert_eq!( + got, + Some(CellValue::Error("circular".into())), + "against it, the bound of 16 passes" + ); + let ran = differential_in(&model, &[], &[], sorted, &[]); + assert!(ran.is_err(), "a sorted stem list must be a divergence"); +} + +type Change = Box; + +/// Another client's writes: Product A's revenue and cost move, and Product C appears. +fn another_clients_writes() -> Change { + Box::new(|remote| { + remote.write(key("East", "A", "Revenue"), Some(CellValue::Number(50.0))); + remote.write(key("East", "A", "Cost"), Some(CellValue::Number(0.0))); + remote.write(key("East", "C", "Revenue"), Some(CellValue::Number(7.0))); + remote.write(key("East", "C", "Cost"), Some(CellValue::Number(2.0))); + }) +} + +fn profit_of(product: &str) -> CellKey { + CellKey::new(vec![ + ("Product".into(), product.into()), + ("_Measure".into(), "Profit".into()), + ]) +} + +#[test] +fn writes_between_two_queries_of_a_refresh_are_healed_by_the_fixed_point() { + let none = vec!["Region".to_string()]; + let armed = Arc::new(AtomicBool::new(false)); + let hook = ChangesAfterAFold { + armed: armed.clone(), + change: another_clients_writes(), + }; + let none_cats = vec!["Region".to_string()]; + let mut pushed = Pushed::through(&hand_model(), none_cats, |r| Hooked(r, hook)); + assert_eq!( + pushed.pd.value(&profit_of("A"), Aggregation::OverHidden), + Some(CellValue::Number(9.0)) + ); + armed.store(true, Ordering::SeqCst); + pushed.pd.refresh(); + assert!( + !armed.load(Ordering::SeqCst), + "the change happened mid-refresh" + ); + let mut after = hand_model(); + after.set_cell(key("East", "A", "Revenue"), CellValue::Number(50.0)); + after.set_cell(key("East", "A", "Cost"), CellValue::Number(0.0)); + let truth = Eager::new(after, none); + let want = truth.value(&profit_of("A"), Aggregation::OverHidden); + assert_eq!(want, Some(CellValue::Number(53.0))); + let got = pushed.pd.value(&profit_of("A"), Aggregation::OverHidden); + assert_eq!( + got, want, + "the last pass read the new state: 55 - 2, and not 13" + ); +} + +#[test] +fn a_stem_that_appears_between_the_stem_list_and_the_folds_waits_for_the_next_refresh() { + let armed = Arc::new(AtomicBool::new(false)); + let hook = ChangesAfterStems { + armed: armed.clone(), + change: another_clients_writes(), + }; + let none_cats = vec!["Region".to_string()]; + let mut pushed = Pushed::through(&hand_model(), none_cats, |r| Hooked(r, hook)); + armed.store(true, Ordering::SeqCst); + pushed.pd.refresh(); + assert_eq!( + pushed.pd.value(&profit_of("C"), Aggregation::OverHidden), + None, + "the stem list was taken before Product C had cells" + ); + pushed.pd.refresh(); + assert_eq!( + pushed.pd.value(&profit_of("C"), Aggregation::OverHidden), + Some(CellValue::Number(5.0)), + "and the next refresh has it" + ); +} + +#[test] +#[should_panic(expected = "cell edits go through the source")] +fn a_cell_edit_on_a_pushdown_is_refused() { + let mut pushed = Pushed::new(&hand_model(), vec![]); + let edit = Edit::SetCell(key("East", "A", "Revenue"), CellValue::Number(1.0)); + pushed.pd.define(&edit); +} + +#[test] +fn a_pushdown_can_be_built_over_the_exported_source_alias() { + let model = hand_model(); + let rows = model.data.iter_cells().map(|(k, v)| (k, v.clone())); + let source: SharedSource = Arc::new(Remote::of(rows)); + let pd = Pushdown::new(model, source, vec![]); + let revenue = CellKey::new(vec![("_Measure".into(), "Revenue".into())]); + assert_eq!(pd.raw_fold(&revenue), Some(CellValue::Number(23.0))); +} diff --git a/crates/improvise-incremental/tests/support/remote.rs b/crates/improvise-incremental/tests/support/remote.rs index adce47c5..b3065698 100644 --- a/crates/improvise-incremental/tests/support/remote.rs +++ b/crates/improvise-incremental/tests/support/remote.rs @@ -6,6 +6,7 @@ use improvise_core::model::cell::{CellKey, CellValue}; use improvise_core::model::source::{CellSource, Select, Summary, TableSource}; use improvise_core::model::{Aggregation, Model}; use improvise_incremental::{Derived, Edit, Maintained, Pushdown, Reading}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; /// What the engine was asked, and how many rows it sent back. @@ -17,7 +18,7 @@ pub struct Asked { pub stem_lists: u64, pub key_lists: u64, pub cell_lists: u64, - /// Keys and cells sent back by `keys` and `cells`: rows, not aggregates. + /// Keys, cells and stems sent back by `keys`, `cells` and `distinct_without`: rows, not aggregates. pub rows_sent: u64, } @@ -151,6 +152,94 @@ impl CellSource for IgnoresHidden { } } +/// What a source does besides answering: reorders the stems it lists, or acts after a query. +pub trait Hook: Send + Sync { + fn stems(&self, stems: Vec, _remote: &Remote) -> Vec { + stems + } + + fn after_summary(&self, _remote: &Remote) {} +} + +/// A source that answers from `remote` and consults `hook`. +pub struct Hooked(pub Arc, pub H); + +impl CellSource for Hooked { + fn value_at(&self, key: &CellKey) -> Option { + self.0.value_at(key) + } + + fn summarize(&self, select: &Select) -> Summary { + let summary = self.0.summarize(select); + self.1.after_summary(&self.0); + summary + } + + fn keys<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + self.0.keys(partial) + } + + fn cells<'a>( + &'a self, + partial: &'a [(String, String)], + ) -> Box + 'a> { + self.0.cells(partial) + } + + fn count(&self, select: &Select, excluding: &[(String, String)]) -> usize { + self.0.count(select, excluding) + } + + fn distinct_without(&self, without: &[String]) -> Vec { + let stems = self.0.distinct_without(without); + self.1.stems(stems, &self.0) + } +} + +/// Lists the stems sorted, not in the order the cells were first seen. +pub struct SortsStems; + +impl Hook for SortsStems { + fn stems(&self, mut stems: Vec, _remote: &Remote) -> Vec { + stems.sort(); + stems + } +} + +type Change = Box; + +/// Changes the table once, when `armed`, right after the stem list is sent. +pub struct ChangesAfterStems { + pub armed: Arc, + pub change: Change, +} + +impl Hook for ChangesAfterStems { + fn stems(&self, stems: Vec, remote: &Remote) -> Vec { + if self.armed.swap(false, Ordering::SeqCst) { + (self.change)(remote); + } + stems + } +} + +/// Changes the table once, when `armed`, right after the first fold is answered. +pub struct ChangesAfterAFold { + pub armed: Arc, + pub change: Change, +} + +impl Hook for ChangesAfterAFold { + fn after_summary(&self, remote: &Remote) { + if self.armed.swap(false, Ordering::SeqCst) { + (self.change)(remote); + } + } +} + /// A `Pushdown` over a remote table that holds the model's cells; the model keeps no store. /// A cell edit is a write to the table, as another client's would be, then a refresh. pub struct Pushed { From 8ba4bd0a6cfee1698cd05bb1403d2a3fbc2c9023 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 19:30:22 -0700 Subject: [PATCH 11/14] 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 7f42a724f982975bc705b24e84e33089d37dd959 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 19:32:39 -0700 Subject: [PATCH 12/14] fix(core): a model put over a source drops the formula cells its own cells computed (improvise-rnak) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/source.md | 2 +- context/test-evidence/core.md | 8 +++++ crates/improvise-core/src/model/over_tests.rs | 34 +++++++++++++++++++ crates/improvise-core/src/model/types.rs | 3 +- 4 files changed, 45 insertions(+), 2 deletions(-) diff --git a/context/code/source.md b/context/code/source.md index 5c369595..b002796e 100644 --- a/context/code/source.md +++ b/context/code/source.md @@ -68,7 +68,7 @@ cells it never owns. 1. **A model over a source is read-only, in every build.** `set_cell`, `clear_cell`, `remove_category` and `remove_item` `assert!` that no source is set (a `debug_assert` first shipped and was lost in release: the write went to the owned store, which nothing reads). A caller who holds a source writes to the - source (ADR 003: the trait has no writes). `Model::over` drops the model's own cells, so no stale copy + source (ADR 003: the trait has no writes). `Model::over` drops the model's own cells and the formula cells computed from them, so no stale copy is left for `get_cell`, persistence or the harness to read. `data` is `pub`, so a caller can still write it directly; it is not read, and nothing prevents that. Structural edits (a category or item removed) are refused too, because the source's cells would still carry them. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 3b70569a..76e01888 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -2442,3 +2442,11 @@ source holds, and the evaluator still reads the source's value. Review of #357: a records-mode `GridLayout` over a source. `group_records` reading the owned store left all 563 tests green; with this test it fails (the grid has no rows). + +### putting_a_model_with_formula_cells_over_a_source_drops_what_those_cells_computed + +Review of #357 (Codex): `Model::over` emptied the store and kept `formula_cache`, which was computed from +those cells; reads consult the cache first, so old formula values survived until a recompute. The model +has a computed Profit, the source holds a different revenue, and the cell must be gone after `over`. +Rival: the `formula_cache.clear()` removed fails this test alone (before the fix it failed here first). +`measure_agg` and the categories are the definition and stay; nothing else is derived from the cells. diff --git a/crates/improvise-core/src/model/over_tests.rs b/crates/improvise-core/src/model/over_tests.rs index 305e8822..1f9edf6a 100644 --- a/crates/improvise-core/src/model/over_tests.rs +++ b/crates/improvise-core/src/model/over_tests.rs @@ -286,3 +286,37 @@ fn the_records_a_drill_freezes_are_read_from_the_source() { } assert!(rows > 10, "only {rows} records were compared"); } + +#[test] +fn putting_a_model_with_formula_cells_over_a_source_drops_what_those_cells_computed() { + let mut owned = fixture(); + owned.recompute_formulas(&["Region".to_string()]); + let profit = CellKey::new(vec![ + ("Product".into(), "A".into()), + ("_Measure".into(), "Profit".into()), + ]); + let stale = owned.evaluate_aggregated(&profit, Aggregation::OverHidden); + assert!( + matches!(stale, Some(CellValue::Number(_))), + "the cache holds Profit" + ); + let revenue = key("East", "A", "Revenue"); + let mut rows: Vec<_> = owned + .data + .iter_cells() + .map(|(k, v)| (k, v.clone())) + .collect(); + rows.iter_mut() + .filter(|(k, _)| *k == revenue) + .for_each(|(_, v)| *v = CellValue::Number(1000.0)); + let over = owned.clone().over(Arc::new(TableSource { rows })); + assert_eq!( + over.formula_cell(&profit), + None, + "computed from cells that are gone" + ); + assert_ne!( + over.evaluate_aggregated(&profit, Aggregation::OverHidden), + stale + ); +} diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 0e5c0193..7aa032bf 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -431,9 +431,10 @@ impl Model { } /// This model, reading its raw cells from `source` and not `data`: read-only. Its own - /// cells are dropped, so no stale second copy is left to read. Why: `context/code/source.md`. + /// cells are dropped, and the formula cells computed from them, so no stale copy is left to read. Why: `context/code/source.md`. pub fn over(mut self, source: SharedSource) -> Self { self.data = DataStore::new(); + self.formula_cache.clear(); self.external = Some(source); self } From ed447c8dc9bbf9e998dd724b88923dc7bc91fa0b Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 19:35:59 -0700 Subject: [PATCH 13/14] docs(context): stem order is a known evaluator defect, not a source contract; define refuses in every build (improvise-rnak) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/improvise-incremental.md | 21 +++++++++++-------- context/code/source.md | 8 ++++--- .../test-evidence/improvise-incremental.md | 4 ++-- .../improvise-incremental/tests/pushdown.rs | 2 +- 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/context/code/improvise-incremental.md b/context/code/improvise-incremental.md index 756cfda0..986b9ee4 100644 --- a/context/code/improvise-incremental.md +++ b/context/code/improvise-incremental.md @@ -457,7 +457,7 @@ change log, no stems. These are the readings taken. members; the view's hidden pairs go as `excluding`. `Eager::occupancy` lists every key to say the same thing. 5. **`define(edit)` takes a formula or an aggregation edit and refreshes.** A cell edit is the source's: - `Model::set_cell` refuses it in a debug build. + `define` refuses it with an `assert!` of its own in every build (`a_cell_edit_on_a_pushdown_is_refused`). 6. **What the differential proves, and what it does not.** `value`, `raw_fold`, `covered` and `total` are the same four functions for `Eager` and `Pushdown` (`reads.rs`; they were copies, and are shared now), so a Pushdown-against-Eager differential cannot see a mistake in them. `Pushdown::members` repeats @@ -473,14 +473,17 @@ change log, no stems. These are the readings taken. that appears after the stem list was taken has no cell until the next refresh (`a_stem_that_appears_between_the_stem_list_and_the_folds_waits_for_the_next_refresh`). A real source should serve a refresh from one read transaction; nothing here can ask it to. -8. **The order of the stems is part of the contract.** The passes of the fixed point visit stems in the - order `distinct_without` lists them, and a document whose stems read each other settles in as many - passes as its dependency chain runs against that order, with a bound of sixteen. A chain of - eighteen stems settles in one pass listed along the chain and not at all listed against it - (`the_order_stems_are_listed_in_decides_a_result_and_a_source_must_keep_it`). No fixture before it - had a result that depended on the order, and a source listing stems sorted passed everything. - The first-seen order is `DataStore`'s insertion order; a source with no order of its own must - fix one. +8. **Known evaluator defect: the result can depend on the order of the stems** (improvise-bw4p.30). The + passes of the fixed point update the cache in place, visiting stems in the order `distinct_without` + lists them, and stop at sixteen. A document whose stems read each other settles in one pass listed + along its dependency chain and takes one pass per link listed against it, so an eighteen-link chain + is `circular` the second way. That is the evaluator's fault, not an obligation on a source: a SQL + source has no order to keep, and bw4p.30 is to make the fixed point independent of it. Until then + the first-seen order (`DataStore`'s insertion order) is what the oracle uses, and + `the_order_stems_are_listed_in_decides_a_result_today` pins today's behaviour: along the chain 18, + sorted `circular`, and a source listing its stems sorted is a divergence. No fixture before it had a + result that depended on the order. When bw4p.30 lands the test's sorted half must change to expect + agreement, and this item goes. 9. **What is and is not proven.** The tests run against `Remote` (`tests/support/remote.rs`), a scanning in-memory table with counters, that shares no evaluator with `Eager` and answers queries over rows. Without a real SQL engine only the **contract** is proven: that a source answering these six diff --git a/context/code/source.md b/context/code/source.md index 8a6507c4..5eb7d12f 100644 --- a/context/code/source.md +++ b/context/code/source.md @@ -110,9 +110,11 @@ engine answers without sending rows (`COUNT(*) ... WHERE`, `SELECT DISTINCT`), a read that would have transferred every key: `Eager::occupancy` lists all keys, and `recompute_formulas` walked every cell to find its stems. `recompute_formulas` now asks `distinct_without(target and None categories)` once per formula, and the order it returns is -the order the cells were first seen in, so the stems are inserted in the order they always were -(the passes of a fixed point visit stems in that order). A source with no order of its own must -fix one, or the passes of a document whose stems read each other may differ between refreshes. +the order the cells were first seen in, so the stems are inserted in the order they always were. +That order is not a contract a source can be held to: the passes of the fixed point visit stems in +it and a chain of stems can settle or not by it. That is a known evaluator defect, improvise-bw4p.30 +(`context/code/improvise-incremental.md`, step 7 item 8), and until it is fixed a source with no order +of its own should list stems in a stable one. `Select::admits` is the predicate of a select over one key, for a source that must test keys itself (`DataStore::count`). `TableSource` does not use it: its own `holds` is the independent second reading. diff --git a/context/test-evidence/improvise-incremental.md b/context/test-evidence/improvise-incremental.md index 2cab469e..abb3b697 100644 --- a/context/test-evidence/improvise-incremental.md +++ b/context/test-evidence/improvise-incremental.md @@ -925,10 +925,10 @@ The same for `ClearCell`. Review of #358: two backends over one feed; one write; `a.sync(); b.sync()`. The first sees it, the second does not. This pins the behaviour as documented rather than as a guarantee: a drain is destructive. -### the_order_stems_are_listed_in_decides_a_result_and_a_source_must_keep_it +### the_order_stems_are_listed_in_decides_a_result_today Review of #360: stem order was invisible to every differential (a sorted `distinct_without` went -uncaught). Eighteen products are stored in chain order with names that sort the other way, and one nested +uncaught). It pins today's behaviour of a known evaluator defect (improvise-bw4p.30), not a contract. Eighteen products are stored in chain order with names that sort the other way, and one nested `IF` formula reads the product before it. Along the chain it settles in one pass (the last link is 18); sorted it needs one pass per link and the bound of sixteen leaves it `circular`. The test asserts both values, that a sound source passes the differential and that a source listing the stems sorted is a diff --git a/crates/improvise-incremental/tests/pushdown.rs b/crates/improvise-incremental/tests/pushdown.rs index 566ee079..8db03a51 100644 --- a/crates/improvise-incremental/tests/pushdown.rs +++ b/crates/improvise-incremental/tests/pushdown.rs @@ -232,7 +232,7 @@ fn chain() -> Model { } #[test] -fn the_order_stems_are_listed_in_decides_a_result_and_a_source_must_keep_it() { +fn the_order_stems_are_listed_in_decides_a_result_today() { let model = chain(); let last = CellKey::new(vec![ ("Product".into(), "n82".into()), From f7404778b003eeb1598570ad3d80f6db2ceb94ed Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 19:48:34 -0700 Subject: [PATCH 14/14] 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"