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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
Claude (dbsp-sources)
2026-10-10 19:00:51 -07:00
committed by Edward Langley
parent 88b644ff2d
commit 82fbe16ea4
6 changed files with 122 additions and 18 deletions
+7 -3
View File
@@ -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 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 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. 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 2. **`Incremental::over(model, feed, none_cats)`** takes a `SharedFeed`, the alias core exports (a generic
keeps no cells of its own. Its categories, formulas and aggregations are the definition; the `Arc<S>` 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. 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 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 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) 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` 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. 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 5. **What a change costs** is what it costs for the owned store: two group updates and two exact-sum
+4 -1
View File
@@ -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` 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 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 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 ## Building a `Summary` from an engine's answer
@@ -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 `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 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. 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<S>(Arc<S>)` 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.
+2 -2
View File
@@ -192,8 +192,8 @@ impl Summary {
/// its readers (a database) does so through interior mutability of its own. /// its readers (a database) does so through interior mutability of its own.
pub type SharedSource = std::sync::Arc<dyn CellSource + Send + Sync>; pub type SharedSource = std::sync::Arc<dyn CellSource + Send + Sync>;
/// A source that reports its changes. Its cells are the state after the changes it has drained, /// A source that reports its changes; its cells are the state as of its last drain.
/// so a snapshot read now plus the changes drained later counts no change twice or not at all. /// A drain is destructive: a feed has one drainer (`context/code/source.md`).
pub trait ChangeSource: CellSource { pub trait ChangeSource: CellSource {
/// The changes since the last drain, oldest first; reads see them from now on. /// The changes since the last drain, oldest first; reads see them from now on.
fn drain_changes(&self) -> Vec<CellChange>; fn drain_changes(&self) -> Vec<CellChange>;
@@ -9,11 +9,10 @@ use crate::{Derived, Edit, Maintained, Reading, cell_delta};
use improvise_core::formula::ast::AggFunc; use improvise_core::formula::ast::AggFunc;
use improvise_core::model::category::MEASURE_CATEGORY; use improvise_core::model::category::MEASURE_CATEGORY;
use improvise_core::model::cell::{CellChange, CellKey, CellValue}; 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::types::{Covers, settled};
use improvise_core::model::{Aggregation, Model}; use improvise_core::model::{Aggregation, Model};
use std::collections::BTreeMap; 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 /// 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. /// 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 /// 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. /// store is not read. Reasoning: `context/code/improvise-incremental.md`, step 7.
pub fn over<S: ChangeSource + Send + Sync + 'static>( pub fn over(model: Model, feed: SharedFeed, none_cats: Vec<String>) -> Self {
model: Model, let source: SharedSource = feed.clone();
feed: Arc<S>, let model = model.over(source);
none_cats: Vec<String>,
) -> Self {
let model = model.over(feed.clone());
Self::start(model, none_cats, Some(feed)) Self::start(model, none_cats, Some(feed))
} }
@@ -390,6 +386,11 @@ impl Derived for Incremental {
impl Maintained for Incremental { impl Maintained for Incremental {
fn apply(&mut self, edit: &Edit) { 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); edit.apply(&mut self.model);
let changes = self.drain(); let changes = self.drain();
self.absorb(Some(edit), &changes); self.absorb(Some(edit), &changes);
@@ -5,12 +5,14 @@ mod support;
use improvise_core::formula::ast::AggFunc; use improvise_core::formula::ast::AggFunc;
use improvise_core::model::cell::{CellKey, CellValue}; 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_core::model::{Aggregation, Model};
use improvise_incremental::{ 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}; use support::{corpus, edits_for, hand_model, key};
#[test] #[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 { fn table(n: usize) -> Model {
let mut model = Model::new("table"); let mut model = Model::new("table");
for (cat, prefix, count) in [("Region", "r", n), ("Product", "p", n), ("Month", "m", 6)] { 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!(fed.inc.raw_fold(&revenue), before);
assert_eq!(before, Some(CellValue::Number(23.0))); 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"
);
}