Merge pull request #344 from fiddlerwoaroof/adr009/cell-delta

feat: ADR 009 step 3, the cell delta, the record tier and the dirty flag (improvise-5r8c)
This commit is contained in:
Ed Langley
2026-10-10 13:57:13 -07:00
committed by GitHub
15 changed files with 814 additions and 91 deletions
Generated
+2
View File
@@ -907,6 +907,7 @@ dependencies = [
"improvise-core",
"improvise-formula",
"improvise-gp",
"improvise-incremental",
"improvise-io",
"improvise-protocol",
"serde",
@@ -959,6 +960,7 @@ dependencies = [
"improvise-core",
"improvise-io",
"proptest",
"zset",
]
[[package]]
+67
View File
@@ -103,6 +103,16 @@ rec {
# File a bug if you depend on any for non-debug work!
debug = internal.debugCrate { inherit packageId; };
};
"improvise-incremental" = rec {
packageId = "improvise-incremental";
build = internal.buildRustCrateWithFeatures {
packageId = "improvise-incremental";
};
# Debug support which might change between releases.
# File a bug if you depend on any for non-debug work!
debug = internal.debugCrate { inherit packageId; };
};
"improvise-io" = rec {
packageId = "improvise-io";
build = internal.buildRustCrateWithFeatures {
@@ -159,6 +169,16 @@ rec {
packageId = "numtower";
};
# Debug support which might change between releases.
# File a bug if you depend on any for non-debug work!
debug = internal.debugCrate { inherit packageId; };
};
"zset" = rec {
packageId = "zset";
build = internal.buildRustCrateWithFeatures {
packageId = "zset";
};
# Debug support which might change between releases.
# File a bug if you depend on any for non-debug work!
debug = internal.debugCrate { inherit packageId; };
@@ -2974,6 +2994,10 @@ rec {
name = "improvise-gp";
packageId = "improvise-gp";
}
{
name = "improvise-incremental";
packageId = "improvise-incremental";
}
{
name = "improvise-io";
packageId = "improvise-io";
@@ -3131,6 +3155,35 @@ rec {
}
];
};
"improvise-incremental" = rec {
crateName = "improvise-incremental";
version = "0.1.0";
edition = "2024";
src = lib.cleanSourceWith { filter = sourceFilter; src = ./crates/improvise-incremental; };
libName = "improvise_incremental";
license = "Apache-2.0";
dependencies = [
{
name = "improvise-core";
packageId = "improvise-core";
}
{
name = "zset";
packageId = "zset";
}
];
devDependencies = [
{
name = "improvise-io";
packageId = "improvise-io";
}
{
name = "proptest";
packageId = "proptest";
}
];
};
"improvise-io" = rec {
crateName = "improvise-io";
@@ -9566,6 +9619,20 @@ rec {
"no-panic" = [ "dep:no-panic" ];
};
};
"zset" = rec {
crateName = "zset";
version = "0.1.0";
edition = "2024";
src = lib.cleanSourceWith { filter = sourceFilter; src = ./crates/zset; };
license = "Apache-2.0";
devDependencies = [
{
name = "proptest";
packageId = "proptest";
}
];
};
};
#
+30 -18
View File
@@ -1267,26 +1267,38 @@ gotcha 1: renaming a category coordinate in wide records mode moves *every*
measure cell in the row, so one legal edit spans a whole record. **A unit finer
than the coarsest legal write cannot be maintained.**
### `record_fingerprints` — a process-local XOR fold
### The record tier and the dirty flag read the cell delta (improvise-5r8c)
A fingerprint per record of the raw cells it holds, keyed by the same surrogate
the index and the version counter are keyed by. One `u64` folded over each cell's
key and value. The fold is XOR, so it does not care what order `iter_cells`
yields — and it cannot cancel a pair against itself, because a key appears at
most once in the store.
`record_fingerprints` (a per-record XOR of cell hashes) and `document_content` (a hash of
every cell) are gone: both walked the whole store per batch. `apply_effects` now turns on
`DataStore::record_changes` for any batch that touches the model, drains the log after
each `CellData` effect into a `CellDelta` (`improvise-incremental`, ADR 009 clause 3), and
at the end of the batch drains what is left. The tail matters: a geometry effect such as
`RemoveCategory` removes cells, and no later `CellData` effect may be there to drain them.
This hasher is **not** the surrogate's: it is process-local, compared only against
a fingerprint taken moments earlier in the same process, and never crosses a wire.
`RecordHash` is the opposite case and is specified accordingly
(`improvise_core::model::record`).
The value hash is exhaustive and by **bits** rather than by `Debug`: two values
that print alike must not fingerprint alike, and it runs once per cell per editing
batch.
Only *raw* data is fingerprinted. Formula results live in the model's separate
`formula_cache`, and a record whose derived value moved because someone edited a
different record has not itself been written.
- **Record tier.** The batch delta, consolidated across effects, names the records to bump
(`moved_records`). A cell set and set back inside one batch cancels, as the XOR did.
- **Dirty flag.** A `CellData` effect dirties the document when its delta is non-zero or
`document_shape` moved. The shape is the formulas, `document_config` and
`DataStore::reorders`. ADR 009 names only the formula fingerprint; two more are needed.
`AddItem` writes no cell and moves the config. The saved order is the other input no
delta carries, and it moves two ways. `SortData` reorders in place, so the store counts
the sorts that moved a cell. A removal or an insertion moves a cell to the end of the
store even when the delta nets to zero (`RecordRewrite` clears then re-sets a cell, so a
same-item rename), so any logged insertion or removal dirties the document. Only an
in-place overwrite keeps its position and gets the float-rule comparison.
- **Which batches record.** Every batch that touches the model, not only those holding a
`CellData` effect: `RemoveCategory` is `LayoutGeometry` and deletes cells, and a batch of
it alone must still bump the records it empties. The log costs O(changes), so the gate
the old whole-store fingerprint needed is gone. It is switched off at the end of the batch.
- **`WholeWorkbook` beside `CellData`.** No production command emits both in one batch
(`LoadModelCmd`, the headless import and `AcceptWizardModel` are each alone). Were they
mixed, the swapped-in store records nothing, but `WholeWorkbook` declares the document
dirty and moves `document_generation`, which invalidates every record basis anyway.
- **Float rules.** `0.0` over `-0.0`, and one NaN over another, are no longer writes: the
old fingerprint hashed bits and counted them.
- **Cost.** A cell write is O(delta) plus `document_shape`, which is O(formulas and
config), not O(cells).
### `document_generation`, `layout_version`, `cursor_epoch`
+33 -2
View File
@@ -55,8 +55,39 @@ fold of the cells that match a partial key") and nothing else. These are the rea
6. **Values agree by float rules with every NaN one value** (`same_value`), the rule clause 3
states for cell deltas. Signed zero compares equal, so a later `ExactSum` cannot be told
from eager's sum by the sign of a zero here; clause 8 has its own property test for that.
7. **No `zset` dependency yet.** Nothing in this step uses a Z-set. It arrives with the first
backend, as the cell delta of step 3.
7. **The `zset` dependency arrived with step 3**, as the cell delta (below).
## Step 3: the cell delta (improvise-5r8c)
`delta.rs` holds `DeltaValue`, `CellDelta = ZSet<(CellKey, DeltaValue)>`, `cell_delta` and
`moved_records`. `improvise-command`'s `apply_effects` reads them for the record tier and the
dirty flag (`context/code/command.md`). Evidence: `context/test-evidence/improvise-incremental.md`.
These are the readings taken, where ADR 009 clause 3 leaves a choice.
1. **`DataStore` logs, `improvise-incremental` builds the Z-set.** The ADR says the store
"emits a `ZSet`", but core never depends on `zset` (clause 2). `DataStore::record_changes`
turns on a `Vec<CellChange>` log (`None` while off, so an unwatched store keeps nothing),
`drain_changes` empties it and `cell_delta` folds it. The log lives in the store, not in
`Model::set_cell`, so every route that writes a cell is seen, including `RemoveCategory`'s
sweep.
2. **`DeltaValue` canonicalises on construction**: every NaN becomes `f64::NAN`, `-0.0` becomes
`+0.0`, and the order is `total_cmp` on the result, with numbers before text before errors.
The inner value is private, so a non-canonical one cannot be built. Equality is the order's
`Equal`, so `==` and `Ord` agree.
3. **`CellKey` is `Ord` by derivation**: the sorted coordinate pairs, compared as strings.
Clause 3 calls this "the natural order of ids", which ADR 007's id form will change; the
derive then moves with the key type.
4. **The record tier is the set of records a consolidated batch delta names.** A cell written
and written back inside one batch cancels, as the old XOR did.
5. **The dirty flag needs two inputs the ADR does not name.** Besides a non-empty delta and
the formula set, `document_config` stays (an `AddItem` writes no cell and the existing test
requires it dirty) and the saved order joins, which no delta carries: `DataStore::reorders`
counts `SortData`, and any logged insertion or removal dirties too, because a clear-then-set
of one value nets to zero yet moves the cell to the end of the store (a same-item rename).
6. **`document_generation` and the wire are untouched**, and `Eager` and the `Derived` trait
are not used: no backend consumes the delta yet. The differential for this step is the
delta against the store it describes (`the_cell_delta_of_an_edit_is_...`), since there is
nothing to compare with `Eager`.
## What the harness checks
+51
View File
@@ -528,6 +528,10 @@ a wrong answer to no divergence at all.
### a_cell_data_effect_no_raw_cell_records_still_dirties_the_document
improvise-5r8c: the dirty flag now reads the cell delta, and `SortData` is the case no delta
carries (the saved order moved). The first cell-delta version failed this test; the fix is
`DataStore::reorders` in `document_shape`, and removing it fails this test alone.
The guard against the cure improvise-748f **proposed**, which is unsound. The ticket
suggested raising `dirty` off `bump_changed_records`' per-record fingerprint of the
raw cells, on the reading that `CellData` means cells. Five of the ten `CellData`
@@ -7227,3 +7231,50 @@ group per level, where the two keyings coincide.
`z` through a closed Year > Quarter > Month hierarchy, cursor at row 1. Rival:
`toggle_group_collapse` never removing a fold; the final assertion (no fold left
behind once the quarter is closed again) fails.
### ui::app::basis_tiers::a_write_that_compares_equal_by_the_float_rules_changes_nothing
improvise-5r8c. `0.0` over `-0.0`, `-0.0` over `0.0` and three NaN spellings move no record
version and leave the document clean. It failed on the bit fingerprint (`0 -> -0` bumped the
version to 2), the bug the ADR names. Faults: dirty set on every `CellData` effect fails it
and two more.
### ui::app::basis_tiers::a_write_that_differs_by_the_float_rules_still_changes_something
`0.0` over NaN bumps the version and dirties. Fault: dirty read from the shape alone ignores
the delta and fails this and four tests elsewhere.
### ui::effect::tests::review344_same_item_rename_reorders_and_dirties
Review of PR 344. A same-item `RenameRecordCoordinate` clears and re-sets each cell: the net
delta is zero, yet the record moves to the end of the store, so the saved order changes. It
dirtied before the cell delta and did not after. Fails when the dirty flag drops the
insertion-or-removal rule (`reordered`).
### ui::app::basis_tiers::a_category_removal_on_its_own_moves_the_records_it_emptied
Codex and Copilot on PR 344. `RemoveCategory` alone is `LayoutGeometry`; the change log was
on only for batches holding a `CellData` effect, so no record was bumped (it also failed on
the base). Fails when recording is gated on `writes_cells` again.
### ui::app::basis_tiers::the_change_log_is_off_once_the_batch_is_over
Review of PR 344: replacing `stop_recording_changes` with a no-op passed all 754 tests. A
direct store write after a `SetCell` batch and after a `RemoveCategory` batch must not log.
### ui::app::basis_tiers::cells_removed_by_a_later_geometry_effect_still_move_their_records
`[SetCell, RemoveCategory]`: the removal comes after the last `CellData` drain. Fault: not
reading the log at the end of the batch survived the whole suite until this test, and then
fails it. A fault dropping the per-effect delta from the batch fails ten tests.
### model::cell::data_store::sorting_counts_as_a_reorder_only_when_it_moves_a_cell
A counter bumped on every sort, and one never bumped, each fail it: the second sort of an
already sorted store is the case that separates them from the first.
### model::cell::data_store::a_recording_store_reports_a_new_cell_an_overwrite_and_a_removal
An overwrite logging `old: None` fails it. Removing an absent cell must log nothing, and a
second drain is empty.
@@ -192,3 +192,26 @@ Clause 2 asks for a property test over a random edit sequence. A proptest (256 c
edits of five kinds over the hand-built model) runs `differential` with a candidate that rebuilds
an `Eager` from scratch on every edit. It fails on the `Eager::apply`-skips-recompute fault, which
is the property: maintaining in place equals building afresh.
### delta::tests::overwriting_a_value_with_one_that_compares_equal_is_the_zero_delta
Every pair of NaN spellings (positive, negative, two payloads), `0.0` over `-0.0` and back,
and equal text each give the zero delta. Faults: `DeltaValue::new` not mapping NaN to the
canonical one, and not mapping `-0.0` to `+0.0`; each fails this test and two others.
### delta::tests::numbers_order_by_total_cmp_with_the_canonical_nan_last
Comparing by bits instead of `f64::total_cmp` passes every equality test, since the values
are canonical, and fails only here, where negatives must sort below positives.
### delta::tests::values_that_differ_by_the_float_rules_stay_different
Guards the other direction: a `cmp` returning `Equal` across variants (`0` against `"0"`)
fails this and the totality test. A weight of `+1` for the old value fails four tests.
### the_cell_delta_of_an_edit_is_the_store_after_minus_the_store_before
The differential for step 3. A proptest (256 cases, up to 12 edits drawn from sets, clears,
both zeros and three NaN spellings) checks that the delta `DataStore` logs for an edit equals
the store as a Z-set after it minus the store before. No `Derived` backend exists yet, so
this is the harness's step-3 form: the delta against the store it describes.
+2
View File
@@ -12,6 +12,8 @@ improvise-protocol = { path = "../improvise-protocol" }
improvise-formula = { path = "../improvise-formula" }
improvise-io = { path = "../improvise-io" }
improvise-gp = { path = "../improvise-gp" }
# The cell delta behind the record tier and the dirty flag (ADR 009 step 3).
improvise-incremental = { path = "../improvise-incremental" }
anyhow = "1"
serde = { version = "1", features = ["derive"] }
# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate,
+127 -64
View File
@@ -19,7 +19,7 @@ use crate::command::cmd::CmdContext;
use crate::command::keymap::{Keymap, KeymapSet};
use crate::format::FormatPlan;
use crate::import::wizard::ImportWizard;
use crate::model::cell::{CellKey, CellValue, DataStore};
use crate::model::cell::{CellKey, CellValue};
use crate::persistence;
use crate::ui::effect::{EffectKind, ModelChange, ViewTarget};
use crate::ui::project::Dirty;
@@ -29,6 +29,7 @@ use crate::ui::viewport::{
use crate::view::{GridLayout, View};
use crate::workbook::Workbook;
use improvise_core::model::record::{AggregateHash, RecordHash};
use improvise_incremental::{CellDelta, cell_delta, moved_records};
use improvise_protocol::{CellId, RecordCell};
/// A cell's coordinate tuple with `_Measure` dropped: the unit of atomicity
@@ -165,15 +166,12 @@ fn view_shapes(workbook: &Workbook) -> std::collections::BTreeMap<String, u64> {
.collect()
}
/// `document_config` plus the cells and the formulas. `CellData` is wider than its
/// name (improvise-748f), and only that arm pays the walk — context/code/command.md.
fn document_content(workbook: &Workbook) -> u64 {
/// `document_config` plus the formulas: everything `CellData` can move besides the
/// cells, which the cell delta reports (improvise-5r8c, context/code/command.md).
fn document_shape(workbook: &Workbook) -> u64 {
use std::hash::{Hash, Hasher};
let mut h = std::collections::hash_map::DefaultHasher::new();
for (key, value) in workbook.model.data.iter_cells() {
key.hash(&mut h);
hash_value(value, &mut h);
}
workbook.model.data.reorders().hash(&mut h);
for formula in workbook.model.formulas() {
formula.raw.hash(&mut h);
formula.target.hash(&mut h);
@@ -243,40 +241,6 @@ fn sorted_pairs(
out
}
/// Exhaustive, and by bits rather than by `Debug`: two values that print alike
/// must not fingerprint alike.
fn hash_value(value: &CellValue, hasher: &mut impl std::hash::Hasher) {
use std::hash::Hash;
match value {
CellValue::Number(n) => {
0u8.hash(hasher);
n.to_bits().hash(hasher);
}
CellValue::Text(t) => {
1u8.hash(hasher);
t.hash(hasher);
}
CellValue::Error(e) => {
2u8.hash(hasher);
e.hash(hasher);
}
}
}
/// A per-record XOR fingerprint of the **raw** cells it holds. Process-local and
/// never on a wire, unlike [`RecordHash`] — see `context/code/command.md`.
fn record_fingerprints(data: &DataStore) -> std::collections::HashMap<RecordHash, u64> {
use std::hash::{Hash, Hasher};
let mut out: std::collections::HashMap<RecordHash, u64> = std::collections::HashMap::new();
for (key, value) in data.iter_cells() {
let mut hasher = std::collections::hash_map::DefaultHasher::new();
key.hash(&mut hasher);
hash_value(value, &mut hasher);
*out.entry(RecordHash::of(&key)).or_insert(0) ^= hasher.finish();
}
out
}
/// Resolution against the grid each view is drawn, each derived at most once per
/// borrow of the `App` (improvise-kimw) — `context/code/command.md`.
pub struct Grids<'a> {
@@ -1080,12 +1044,14 @@ impl App {
let moves_geometry = effects
.iter()
.any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::LayoutGeometry)));
let records_before =
writes_cells.then(|| record_fingerprints(&self.model_state.workbook.model.data));
if touches_model {
self.model_state.workbook.model.data.record_changes();
}
let mut batch_delta = CellDelta::new();
let shapes_before = touches_model.then(|| view_shapes(&self.model_state.workbook));
// Two bases, because the cheap one is the held-key path: `[`/`]`, axis
// cycle and hide/show must not walk 18,000 cells (improvise-748f).
let mut content_basis = writes_cells.then(|| document_content(&self.model_state.workbook));
let mut shape_basis = writes_cells.then(|| document_shape(&self.model_state.workbook));
let mut config_basis = moves_geometry.then(|| document_config(&self.model_state.workbook));
// The cursor's own basis. The cheap one-view form on the held-key path,
// and every view where a cell write can prune one — `context/code/command.md`.
@@ -1116,11 +1082,17 @@ impl App {
// The observed half, read **inside the loop** so `Save` still clears it
// in loop order (gotcha 45) — the position is the property.
if matches!(kind, EffectKind::Model(ModelChange::CellData))
&& let Some(basis) = content_basis
&& let Some(basis) = shape_basis
{
let now = document_content(&self.model_state.workbook);
self.model_state.dirty |= now != basis;
content_basis = Some(now);
let changes = self.model_state.workbook.model.data.drain_changes();
let delta = cell_delta(&changes);
// An insertion or a removal moves the saved order even when the
// delta nets to zero (a clear-then-set of one value).
let reordered = changes.iter().any(|c| c.old.is_none() || c.new.is_none());
let now = document_shape(&self.model_state.workbook);
self.model_state.dirty |= reordered || !delta.is_zero() || now != basis;
batch_delta += delta;
shape_basis = Some(now);
}
if matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry))
&& let Some(basis) = config_basis
@@ -1174,8 +1146,11 @@ impl App {
break;
}
}
if let Some(before) = records_before {
self.bump_changed_records(&before);
if touches_model {
let data = &mut self.model_state.workbook.model.data;
batch_delta += cell_delta(&data.drain_changes());
data.stop_recording_changes();
self.bump_moved_records(&batch_delta);
}
self.observe_written_view_frames();
self.observe_reshape(shapes_before);
@@ -1203,20 +1178,11 @@ impl App {
}
}
/// Bump every record whose raw cells read differently than at `before`. Gained,
/// lost and rewritten all count — each moves under an older basis.
fn bump_changed_records(&mut self, before: &std::collections::HashMap<RecordHash, u64>) {
/// Bump every record the batch's cell delta touches. Gained, lost and rewritten
/// all count, each moves under an older basis; a record is bumped once.
fn bump_moved_records(&mut self, delta: &CellDelta) {
let data = &mut self.model_state.workbook.model.data;
let after = record_fingerprints(data);
// A set, not a list: a record present in both maps is named twice by the
// chain, and a rewritten record would otherwise count as two writes.
let moved: std::collections::HashSet<RecordHash> = before
.keys()
.chain(after.keys())
.filter(|record| before.get(*record) != after.get(*record))
.copied()
.collect();
for record in moved {
for record in moved_records(delta) {
data.bump_record(record);
}
}
@@ -5228,6 +5194,103 @@ mod basis_tiers {
assert_eq!(app.record_version(&record("North")), 0);
}
/// Clause 3 of ADR 009: values compare by the float rules, so `-0.0` over
/// `0.0` and one NaN over another are not changes (the old scan hashed bits).
#[test]
fn a_write_that_compares_equal_by_the_float_rules_changes_nothing() {
let same = [
(0.0, -0.0),
(-0.0, 0.0),
(f64::NAN, -f64::NAN),
(f64::NAN, f64::from_bits(0x7ff0_0000_0000_0001)),
];
for (was, now) in same {
let mut app = app_with_two_records();
let north = cell("North", "Revenue");
app.apply_effects(vec![Box::new(SetCell(
north.clone(),
CellValue::Number(was),
))]);
app.model_state.dirty = false;
let version = app.record_version(&record("North"));
assert_eq!(version, 1, "the fixture write must register");
app.apply_effects(vec![Box::new(SetCell(north, CellValue::Number(now)))]);
assert_eq!(
app.record_version(&record("North")),
version,
"{was} -> {now}"
);
assert!(!app.model_state.dirty, "{was} -> {now}");
}
}
/// Cells a non-`CellData` effect removes, in a batch that also writes cells, are
/// read at the end of the batch: no later `CellData` effect drains them.
#[test]
fn cells_removed_by_a_later_geometry_effect_still_move_their_records() {
let mut app = app_with_two_records();
app.apply_effects(vec![
Box::new(SetCell(cell("North", "Revenue"), CellValue::Number(99.0))),
Box::new(crate::ui::effect::RemoveCategory("Month".into())),
]);
assert_eq!(app.model_state.workbook.model.data.iter_cells().count(), 0);
assert_eq!(app.record_version(&record("South")), 1, "lost both cells");
}
/// A geometry-only batch removes cells too, so it must bump the records it empties.
#[test]
fn a_category_removal_on_its_own_moves_the_records_it_emptied() {
let mut app = app_with_two_records();
app.apply_effects(vec![Box::new(crate::ui::effect::RemoveCategory(
"Month".into(),
))]);
assert_eq!(app.model_state.workbook.model.data.iter_cells().count(), 0);
assert_eq!(app.record_version(&record("North")), 1);
assert_eq!(app.record_version(&record("South")), 1);
}
/// The change log is on for a batch only: a store write after it is not logged.
#[test]
fn the_change_log_is_off_once_the_batch_is_over() {
for effects in [
vec![
Box::new(SetCell(cell("North", "Revenue"), CellValue::Number(1.0)))
as Box<dyn crate::ui::effect::Effect>,
],
vec![Box::new(crate::ui::effect::RemoveCategory("Month".into()))],
] {
let mut app = app_with_two_records();
app.apply_effects(effects);
let data = &mut app.model_state.workbook.model.data;
data.set(cell("West", "Revenue"), CellValue::Number(5.0));
assert!(data.drain_changes().is_empty(), "the log was left on");
}
}
/// The other direction, which the equal-value test cannot see: a real
/// change still dirties the document and moves the record.
#[test]
fn a_write_that_differs_by_the_float_rules_still_changes_something() {
let mut app = app_with_two_records();
let north = cell("North", "Revenue");
app.apply_effects(vec![Box::new(SetCell(
north.clone(),
CellValue::Number(0.0),
))]);
app.model_state.dirty = false;
app.apply_effects(vec![Box::new(SetCell(north, CellValue::Number(f64::NAN)))]);
assert_eq!(app.record_version(&record("North")), 2);
assert!(app.model_state.dirty);
}
/// The counters are shared document state on `App`, never on the portable
/// session slice (gotcha 30).
#[test]
+35
View File
@@ -3081,6 +3081,41 @@ mod tests {
);
}
/// A record rewrite clears and re-sets each cell, so a same-item rename nets a
/// zero delta yet moves the record to the end of the store: the saved order.
#[test]
fn review344_same_item_rename_reorders_and_dirties() {
let mut app = test_app();
let k = |t: &str| {
CellKey::new(vec![
("Type".into(), t.into()),
("Month".into(), "Jan".into()),
("_Measure".into(), "Revenue".into()),
])
};
app.model_state
.workbook
.model
.set_cell(k("Food"), CellValue::Number(1.0));
app.model_state
.workbook
.model
.set_cell(k("Clothing"), CellValue::Number(2.0));
app.model_state.dirty = false;
let order = |app: &App| -> Vec<CellKey> {
let cells = app.model_state.workbook.model.data.iter_cells();
cells.map(|(k, _)| k).collect()
};
let before = order(&app);
app.apply_effects(vec![Box::new(RenameRecordCoordinate {
record: k("Food").without("_Measure"),
category: "Type".into(),
item: "Food".into(),
})]);
assert_ne!(before, order(&app), "precondition: the saved order moved");
assert!(app.model_state.dirty, "a moved saved order must dirty");
}
/// `CellData` is wider than its name, and a raw-cell fingerprint sees none of
/// these — the cheap cure improvise-748f proposed would have lost them.
#[test]
+133 -4
View File
@@ -7,7 +7,7 @@ use super::symbol::{Symbol, SymbolTable};
/// A cell key: (category, item) pairs, always sorted by category name. The
/// constructors are the only way to build one — context/code/core.md §1.
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct CellKey(pub Vec<(String, String)>);
impl CellKey {
@@ -194,6 +194,15 @@ impl RecordEntry {
}
}
/// One cell's move in the store: `old` is `None` for a new cell, `new` is
/// `None` for a removal. ADR 009 clause 3; `context/code/improvise-incremental.md`.
#[derive(Debug, Clone, PartialEq)]
pub struct CellChange {
pub key: CellKey,
pub old: Option<CellValue>,
pub new: Option<CellValue>,
}
#[cfg(test)]
thread_local! {
/// How many lookups fell through to set narrowing, for the exact-path test.
@@ -215,6 +224,10 @@ pub struct DataStore {
/// How many stored cells have each number of coordinates; its largest key
/// says when a partial can match only one exact cell.
arities: std::collections::BTreeMap<usize, usize>,
/// The changes since `record_changes`, or `None` while nobody listens, so an
/// unwatched store keeps no log.
changes: Option<Vec<CellChange>>,
reorders: u64,
}
/// Resolved through the symbol table and sorted wherever the storage is a
@@ -227,6 +240,8 @@ impl std::fmt::Debug for DataStore {
index,
records,
arities: _,
changes: _,
reorders: _,
} = self;
let cells: Vec<_> = cells
.iter()
@@ -277,6 +292,18 @@ impl DataStore {
/// Sort cells by `CellKey`. Called once on entry into records mode;
/// subsequent inserts append at the end.
pub fn sort_by_key(&mut self) {
let before: Vec<InternedKey> = self.cells.keys().cloned().collect();
self.sort_cells();
self.reorders += u64::from(self.cells.keys().ne(before.iter()));
}
/// How many times `sort_by_key` has moved a cell. Order is saved, so a reorder
/// is a change no [`CellChange`] reports.
pub fn reorders(&self) -> u64 {
self.reorders
}
fn sort_cells(&mut self) {
let symbols = &self.symbols;
self.cells.sort_by(|a, _, b, _| {
let resolve = |k: &InternedKey| -> Vec<(String, String)> {
@@ -302,7 +329,19 @@ impl DataStore {
self.index.entry(*pair).or_default().insert(ikey.clone());
}
let width = ikey.0.len();
let is_new_cell = self.cells.insert(ikey, value).is_none();
let previous = match &mut self.changes {
Some(log) => {
let previous = self.cells.insert(ikey, value.clone());
log.push(CellChange {
key,
old: previous.clone(),
new: Some(value),
});
previous
}
None => self.cells.insert(ikey, value),
};
let is_new_cell = previous.is_none();
if is_new_cell {
*self.arities.entry(width).or_default() += 1;
}
@@ -316,6 +355,24 @@ impl DataStore {
entry.cells += usize::from(is_new_cell);
}
/// Start logging every `set` and `remove` as a [`CellChange`], from empty.
pub fn record_changes(&mut self) {
self.changes = Some(Vec::new());
}
/// Stop logging and drop what was logged.
pub fn stop_recording_changes(&mut self) {
self.changes = None;
}
/// The changes since the last drain, oldest first. Logging goes on.
pub fn drain_changes(&mut self) -> Vec<CellChange> {
self.changes
.as_mut()
.map(std::mem::take)
.unwrap_or_default()
}
pub fn get(&self, key: &CellKey) -> Option<&CellValue> {
let ikey = self.lookup_key(key)?;
self.cells.get(&ikey)
@@ -340,7 +397,14 @@ impl DataStore {
let Some(ikey) = self.lookup_key(key) else {
return;
};
if self.cells.shift_remove(&ikey).is_some() {
if let Some(old) = self.cells.shift_remove(&ikey) {
if let Some(log) = &mut self.changes {
log.push(CellChange {
key: key.clone(),
old: Some(old),
new: None,
});
}
if let Some(n) = self.arities.get_mut(&ikey.0.len()) {
*n -= 1;
if *n == 0 {
@@ -714,6 +778,17 @@ mod cell_key {
assert!(!k.matches_partial(&partial));
}
#[test]
fn keys_order_by_their_sorted_coordinates() {
let a = key(&[("A", "z")]);
let b = key(&[("A", "z"), ("B", "a")]);
let c = key(&[("A", "z"), ("B", "b")]);
let d = key(&[("C", "a")]);
let mut keys = vec![d.clone(), c.clone(), a.clone(), b.clone()];
keys.sort();
assert_eq!(keys, vec![a, b, c, d]);
}
#[test]
fn display_format() {
let k = key(&[("Region", "East")]);
@@ -723,7 +798,7 @@ mod cell_key {
#[cfg(test)]
mod data_store {
use super::{CellKey, CellValue, DataStore};
use super::{CellChange, CellKey, CellValue, DataStore};
fn key(pairs: &[(&str, &str)]) -> CellKey {
CellKey::new(
@@ -757,6 +832,60 @@ mod data_store {
assert_eq!(store.get(&k), Some(&CellValue::Number(99.0)));
}
#[test]
fn a_store_reports_no_changes_until_asked_to_record_them() {
let mut store = DataStore::new();
store.set(key(&[("Region", "East")]), CellValue::Number(1.0));
assert!(store.drain_changes().is_empty());
}
#[test]
fn a_recording_store_reports_a_new_cell_an_overwrite_and_a_removal() {
let mut store = DataStore::new();
let k = key(&[("Region", "East")]);
store.record_changes();
store.set(k.clone(), CellValue::Number(1.0));
store.set(k.clone(), CellValue::Number(2.0));
store.remove(&k);
store.remove(&k);
let change = |old: Option<f64>, new: Option<f64>| CellChange {
key: k.clone(),
old: old.map(CellValue::Number),
new: new.map(CellValue::Number),
};
assert_eq!(
store.drain_changes(),
vec![
change(None, Some(1.0)),
change(Some(1.0), Some(2.0)),
change(Some(2.0), None)
],
"removing an absent cell reports nothing"
);
assert!(store.drain_changes().is_empty(), "a drain empties the log");
}
#[test]
fn sorting_counts_as_a_reorder_only_when_it_moves_a_cell() {
let mut store = DataStore::new();
store.set(key(&[("Region", "West")]), CellValue::Number(1.0));
store.set(key(&[("Region", "East")]), CellValue::Number(2.0));
assert_eq!(store.reorders(), 0);
store.sort_by_key();
assert_eq!(store.reorders(), 1, "West, East became East, West");
store.sort_by_key();
assert_eq!(store.reorders(), 1, "already sorted: nothing moved");
}
#[test]
fn a_store_that_stopped_recording_reports_nothing_more() {
let mut store = DataStore::new();
store.record_changes();
store.stop_recording_changes();
store.set(key(&[("Region", "East")]), CellValue::Number(1.0));
assert!(store.drain_changes().is_empty());
}
#[test]
fn remove_evicts_key() {
let mut store = DataStore::new();
+2 -1
View File
@@ -6,9 +6,10 @@ license = "Apache-2.0"
description = "The seam between improvise-core and the incremental circuit: the Derived trait, the Eager oracle and the differential harness (ADR 009 clause 2)."
# ADR 009 clause 2: depends on improvise-core, never the other way round. The
# `zset` edge arrives with the first backend that uses it (step 3), not before.
# `zset` edge is the cell delta of step 3 (improvise-5r8c).
[dependencies]
improvise-core = { path = "../improvise-core" }
zset = { path = "../zset" }
[dev-dependencies]
improvise-io = { path = "../improvise-io" }
+260
View File
@@ -0,0 +1,260 @@
//! The cell delta: what a write changed, as a Z-set (ADR 009 clause 3).
//! Reasoning: `context/code/improvise-incremental.md`.
use std::cmp::Ordering;
use improvise_core::model::cell::{CellChange, CellKey, CellValue};
use improvise_core::model::record::RecordHash;
use zset::ZSet;
/// A cell value that compares by the float rules, every NaN one value and `-0.0`
/// equal to `+0.0`. Only [`DeltaValue::new`] builds one, so the order is total.
#[derive(Debug, Clone)]
pub struct DeltaValue(CellValue);
impl DeltaValue {
pub fn new(value: &CellValue) -> Self {
DeltaValue(match value {
CellValue::Number(n) if n.is_nan() => CellValue::Number(f64::NAN),
CellValue::Number(n) if *n == 0.0 => CellValue::Number(0.0),
other => other.clone(),
})
}
pub fn value(&self) -> &CellValue {
&self.0
}
}
impl Ord for DeltaValue {
fn cmp(&self, other: &Self) -> Ordering {
match (&self.0, &other.0) {
(CellValue::Number(a), CellValue::Number(b)) => a.total_cmp(b),
(CellValue::Text(a), CellValue::Text(b)) => a.cmp(b),
(CellValue::Error(a), CellValue::Error(b)) => a.cmp(b),
(a, b) => rank(a).cmp(&rank(b)),
}
}
}
fn rank(value: &CellValue) -> u8 {
match value {
CellValue::Number(_) => 0,
CellValue::Text(_) => 1,
CellValue::Error(_) => 2,
}
}
impl PartialOrd for DeltaValue {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
}
}
impl PartialEq for DeltaValue {
fn eq(&self, other: &Self) -> bool {
self.cmp(other) == Ordering::Equal
}
}
impl Eq for DeltaValue {}
/// What the store's cells changed by: `+(k, v)` new, `-(k, v0) + (k, v1)` overwritten,
/// `-(k, v0)` removed, and nothing for an overwrite that changes no value.
pub type CellDelta = ZSet<(CellKey, DeltaValue)>;
/// The delta of `changes`, in order. Changes that cancel leave nothing.
pub fn cell_delta<'a>(changes: impl IntoIterator<Item = &'a CellChange>) -> CellDelta {
let mut delta = CellDelta::new();
for change in changes {
if let Some(old) = &change.old {
delta.insert((change.key.clone(), DeltaValue::new(old)), -1);
}
if let Some(new) = &change.new {
delta.insert((change.key.clone(), DeltaValue::new(new)), 1);
}
}
delta
}
/// The records `delta` touches, ascending: the record tier of ADR 001's basis.
pub fn moved_records(delta: &CellDelta) -> Vec<RecordHash> {
let mut records: Vec<RecordHash> = delta
.iter()
.map(|((key, _), _)| RecordHash::of(key))
.collect();
records.sort_unstable();
records.dedup();
records
}
#[cfg(test)]
mod tests {
use super::*;
fn key(region: &str, measure: &str) -> CellKey {
CellKey::new(vec![
("Region".into(), region.into()),
("_Measure".into(), measure.into()),
])
}
fn change(k: &CellKey, old: Option<CellValue>, new: Option<CellValue>) -> CellChange {
CellChange {
key: k.clone(),
old,
new,
}
}
fn num(n: f64) -> CellValue {
CellValue::Number(n)
}
fn delta_of(old: Option<CellValue>, new: Option<CellValue>) -> CellDelta {
cell_delta(&[change(&key("East", "Revenue"), old, new)])
}
#[test]
fn a_new_cell_an_overwrite_and_a_removal_have_the_clause_3_shapes() {
let k = key("East", "Revenue");
let at = |v: f64| (k.clone(), DeltaValue::new(&num(v)));
assert_eq!(delta_of(None, Some(num(1.0))), ZSet::singleton(at(1.0), 1));
assert_eq!(
delta_of(Some(num(1.0)), Some(num(2.0))),
ZSet::from_iter([(at(1.0), -1), (at(2.0), 1)])
);
assert_eq!(delta_of(Some(num(1.0)), None), ZSet::singleton(at(1.0), -1));
}
#[test]
fn overwriting_a_value_with_one_that_compares_equal_is_the_zero_delta() {
let nans = [
f64::NAN,
-f64::NAN,
f64::from_bits(0x7ff0_0000_0000_0001),
f64::from_bits(0xfff8_dead_beef_0001),
];
for a in nans {
for b in nans {
assert!(
delta_of(Some(num(a)), Some(num(b))).is_zero(),
"NaN {:#x} over NaN {:#x}",
a.to_bits(),
b.to_bits()
);
}
}
assert!(delta_of(Some(num(0.0)), Some(num(-0.0))).is_zero());
assert!(delta_of(Some(num(-0.0)), Some(num(0.0))).is_zero());
let text = || Some(CellValue::Text("x".into()));
assert!(delta_of(text(), text()).is_zero());
}
#[test]
fn values_that_differ_by_the_float_rules_stay_different() {
let pairs = [
(num(1.0), num(1.0000000000000002)),
(num(f64::INFINITY), num(f64::NEG_INFINITY)),
(num(f64::NAN), num(0.0)),
(num(0.0), CellValue::Text("0".into())),
(CellValue::Text("a".into()), CellValue::Error("a".into())),
];
for (a, b) in pairs {
assert!(
!delta_of(Some(a.clone()), Some(b.clone())).is_zero(),
"{a:?} over {b:?}"
);
}
}
#[test]
fn the_order_is_total_and_agrees_with_equality() {
let values = [
num(f64::NEG_INFINITY),
num(-1.0),
num(-0.0),
num(0.0),
num(1.0),
num(f64::INFINITY),
num(f64::NAN),
num(-f64::NAN),
CellValue::Text(String::new()),
CellValue::Text("a".into()),
CellValue::Error("a".into()),
];
let wrapped: Vec<DeltaValue> = values.iter().map(DeltaValue::new).collect();
for a in &wrapped {
for b in &wrapped {
assert_eq!(a.cmp(b), b.cmp(a).reverse());
assert_eq!(a == b, a.cmp(b) == Ordering::Equal);
for c in &wrapped {
if a <= b && b <= c {
assert!(a <= c, "{a:?} {b:?} {c:?}");
}
}
}
}
assert_eq!(wrapped[2], wrapped[3], "-0.0 is +0.0");
assert_eq!(wrapped[6], wrapped[7], "every NaN is one value");
}
#[test]
fn numbers_order_by_total_cmp_with_the_canonical_nan_last() {
let ascending = [f64::NEG_INFINITY, -1.5, -0.0, 2.0, f64::INFINITY, -f64::NAN];
let mut shuffled: Vec<DeltaValue> =
[2.0, -f64::NAN, -1.5, f64::INFINITY, -0.0, f64::NEG_INFINITY]
.iter()
.map(|n| DeltaValue::new(&num(*n)))
.collect();
shuffled.sort();
let want: Vec<DeltaValue> = ascending
.iter()
.map(|n| DeltaValue::new(&num(*n)))
.collect();
assert_eq!(shuffled, want);
assert_eq!(
shuffled[2].value().as_f64().map(f64::to_bits),
Some(0),
"and the zero among them reads as +0.0"
);
}
#[test]
fn a_wrapped_value_reads_back_with_the_canonical_nan_and_zero() {
let nan = DeltaValue::new(&num(-f64::NAN));
assert_eq!(
nan.value().as_f64().map(f64::to_bits),
Some(f64::NAN.to_bits())
);
let zero = DeltaValue::new(&num(-0.0));
assert_eq!(zero.value().as_f64().map(f64::to_bits), Some(0));
}
#[test]
fn a_change_that_undoes_an_earlier_one_cancels_it() {
let k = key("East", "Revenue");
let delta = cell_delta(&[
change(&k, None, Some(num(1.0))),
change(&k, Some(num(1.0)), Some(num(2.0))),
change(&k, Some(num(2.0)), None),
]);
assert!(delta.is_zero());
}
#[test]
fn moved_records_names_each_record_once_whatever_the_measure() {
let delta = cell_delta(&[
change(&key("East", "Revenue"), None, Some(num(1.0))),
change(&key("East", "Cost"), None, Some(num(2.0))),
change(&key("West", "Cost"), Some(num(3.0)), Some(num(4.0))),
change(&key("South", "Cost"), Some(num(5.0)), Some(num(5.0))),
]);
let mut want = vec![
RecordHash::of(&key("East", "Revenue")),
RecordHash::of(&key("West", "Revenue")),
];
want.sort_unstable();
assert_eq!(moved_records(&delta), want, "South wrote nothing new");
}
}
+2
View File
@@ -1,10 +1,12 @@
//! The plug: what the evaluator reads, and the oracle that answers it today.
//! Reasoning: `context/code/improvise-incremental.md`.
mod delta;
mod eager;
mod edit;
mod harness;
pub use delta::{CellDelta, DeltaValue, cell_delta, moved_records};
pub use eager::Eager;
pub use edit::Edit;
pub use harness::{
+46 -1
View File
@@ -7,7 +7,8 @@ use improvise_core::model::cell::{CellKey, CellValue};
use improvise_core::model::{Aggregation, Model};
use improvise_core::view::Axis;
use improvise_incremental::{
Derived, Divergence, Eager, Edit, Maintained, Probe, differential, probes, same_value,
CellDelta, DeltaValue, Derived, Divergence, Eager, Edit, Maintained, Probe, cell_delta,
differential, probes, same_value,
};
use proptest::prelude::*;
use std::path::Path;
@@ -594,6 +595,50 @@ fn any_value() -> impl Strategy<Value = CellValue> {
]
}
/// The store as a Z-set, every cell at weight one.
fn snapshot(model: &Model) -> CellDelta {
model
.data
.iter_cells()
.map(|(k, v)| ((k, DeltaValue::new(v)), 1))
.collect()
}
/// Values the delta must treat as the float rules do: both zeros, several NaNs.
fn delta_value() -> impl Strategy<Value = CellValue> {
prop_oneof![
(-2i32..3).prop_map(|n| CellValue::Number(f64::from(n))),
Just(CellValue::Number(-0.0)),
Just(CellValue::Number(f64::NAN)),
Just(CellValue::Number(-f64::NAN)),
Just(CellValue::Number(f64::from_bits(0x7ff0_0000_0000_0001))),
prop::sample::select(vec!["n", "x"]).prop_map(|t| CellValue::Text(t.into())),
]
}
fn cell_edit() -> impl Strategy<Value = Edit> {
prop_oneof![
(any_key(), delta_value()).prop_map(|(k, v)| Edit::SetCell(k, v)),
any_key().prop_map(Edit::ClearCell),
]
}
proptest! {
#[test]
fn the_cell_delta_of_an_edit_is_the_store_after_minus_the_store_before(
edits in prop::collection::vec(cell_edit(), 0..12)
) {
let mut model = hand_model();
model.data.record_changes();
for edit in &edits {
let before = snapshot(&model);
edit.apply(&mut model);
let delta = cell_delta(&model.data.drain_changes());
prop_assert_eq!(delta, &snapshot(&model) - &before, "after {:?}", edit);
}
}
}
fn any_edit() -> impl Strategy<Value = Edit> {
prop_oneof![
(any_key(), any_value()).prop_map(|(k, v)| Edit::SetCell(k, v)),
+1 -1
View File
@@ -48,7 +48,7 @@
"improvise-gp"
# ADR 005 clause 14: nothing depends on it yet, and improvise-core will.
"numtower"
# ADR 009 clause 1: std-only; nothing depends on it yet.
# ADR 009 clause 1: std-only; improvise-incremental depends on it.
"zset"
# ADR 009 clause 2: the Derived trait, Eager and the differential harness.
"improvise-incremental"