From 315e271905ea55e2979bae25a09636545c6e3517 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:03:20 -0700 Subject: [PATCH 1/3] 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 bebaeb2d61a1009dbe5e85e598558c6608a3f569 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:57:13 -0700 Subject: [PATCH 2/3] 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 7f42a724f982975bc705b24e84e33089d37dd959 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 19:32:39 -0700 Subject: [PATCH 3/3] 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 }