Merge pull request #357 from fiddlerwoaroof/dbsp/model-over-source

feat(core): a model can be over a source, so one evaluator serves any of them (improvise-rnak)
This commit is contained in:
Ed Langley
2026-10-10 20:11:45 -07:00
committed by GitHub
7 changed files with 481 additions and 11 deletions
+26 -8
View File
@@ -56,15 +56,33 @@ 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. 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.
`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<dyn CellSource + Send + Sync>`) 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, 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 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.
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` (a production caller: `improvise-command/src/ui/effect.rs`,
the `old` value of a rename), `Model::data` (`pub`), 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`
+69
View File
@@ -2351,6 +2351,42 @@ A select with `exact` and no span is the cell, not the cells containing the pair
`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.
### 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
@@ -2384,3 +2420,36 @@ version merged one way and a rival rounding only the exact part passed it; Opus
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).
### 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.
+3
View File
@@ -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};
@@ -0,0 +1,322 @@
//! 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<CellKey> {
let mut out: Vec<CellKey> = 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<CellKey> = 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<CellValue>, b: &Option<CellValue>) -> 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]
#[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;
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");
}
#[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
);
}
@@ -209,6 +209,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<dyn CellSource + Send + Sync>;
/// 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 {
+31 -3
View File
@@ -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<String, AggFunc>,
/// Cached formula evaluation results. Recomputed by `recompute_formulas`.
formula_cache: HashMap<CellKey, CellValue>,
/// Where raw cells are read from instead of `data`, for a model over a live source.
external: Option<SharedSource>,
}
/// 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,24 @@ 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. Its own
/// 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
}
/// 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<String>) -> Result<CategoryId> {
@@ -466,6 +487,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;
}
@@ -488,6 +510,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);
}
@@ -538,10 +561,12 @@ impl Model {
}
pub fn set_cell(&mut self, key: CellKey, value: CellValue) {
self.writable();
self.data.set(key, value);
}
pub fn clear_cell(&mut self, key: &CellKey) {
self.writable();
self.data.remove(key);
}
@@ -726,9 +751,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
+26
View File
@@ -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();