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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
committed by
Edward Langley
parent
bcc773bb2a
commit
7f42a724f9
@@ -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
|
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:
|
`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
|
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
|
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)
|
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.
|
are refused too, because the source's cells would still carry them.
|
||||||
|
|||||||
@@ -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
|
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).
|
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.
|
||||||
|
|||||||
@@ -286,3 +286,37 @@ fn the_records_a_drill_freezes_are_read_from_the_source() {
|
|||||||
}
|
}
|
||||||
assert!(rows > 10, "only {rows} records were compared");
|
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
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
@@ -431,9 +431,10 @@ impl Model {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// This model, reading its raw cells from `source` and not `data`: read-only. Its own
|
/// 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 {
|
pub fn over(mut self, source: SharedSource) -> Self {
|
||||||
self.data = DataStore::new();
|
self.data = DataStore::new();
|
||||||
|
self.formula_cache.clear();
|
||||||
self.external = Some(source);
|
self.external = Some(source);
|
||||||
self
|
self
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user