From 9dd78a1a9e4f239612a13fefdb92a8a2bcd74c3c Mon Sep 17 00:00:00 2001 From: "Claude (adr009-step3)" Date: Sat, 10 Oct 2026 11:26:19 -0700 Subject: [PATCH] feat(command): the record tier and the dirty flag read the cell delta (improvise-5r8c) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- Cargo.lock | 2 + context/code/command.md | 37 +++-- context/code/improvise-incremental.md | 34 +++- context/test-evidence/command.md | 34 ++++ .../test-evidence/improvise-incremental.md | 23 +++ crates/improvise-command/Cargo.toml | 2 + crates/improvise-command/src/ui/app.rs | 155 ++++++++++-------- crates/improvise-core/src/model/cell.rs | 26 +++ 8 files changed, 229 insertions(+), 84 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index afad89ba..868ea8fd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -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]] diff --git a/context/code/command.md b/context/code/command.md index 843d0f0d..7d029227 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -1267,26 +1267,27 @@ 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` when a batch holds a `CellData` effect, 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. `SortData` writes no cell either and + moves the saved order, which no delta carries, so the store counts the sorts that moved + a cell. +- **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` diff --git a/context/code/improvise-incremental.md b/context/code/improvise-incremental.md index 35e39331..32df1041 100644 --- a/context/code/improvise-incremental.md +++ b/context/code/improvise-incremental.md @@ -55,8 +55,38 @@ 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` 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 `DataStore::reorders` joins (`SortData` reorders the saved cells and + no delta carries order). +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 diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 08ce94cb..90f265fe 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -7227,3 +7227,37 @@ 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. + +### 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. + +### 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. + +### 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. + +### effect::tests::a_cell_data_effect_no_raw_cell_records_still_dirties_the_document + +Predates this step. `SortData` moves the saved order and no cell delta reports it, so the +first cell-delta version of the dirty flag failed this test. `DataStore::reorders` fixes it, +and removing it from `document_shape` fails this test alone. + +### 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. diff --git a/context/test-evidence/improvise-incremental.md b/context/test-evidence/improvise-incremental.md index aac72cbc..b4a68838 100644 --- a/context/test-evidence/improvise-incremental.md +++ b/context/test-evidence/improvise-incremental.md @@ -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. diff --git a/crates/improvise-command/Cargo.toml b/crates/improvise-command/Cargo.toml index bc1ef6e9..fed6d463 100644 --- a/crates/improvise-command/Cargo.toml +++ b/crates/improvise-command/Cargo.toml @@ -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, diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index ce17b26d..7f5bd1be 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -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 { .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 { - use std::hash::{Hash, Hasher}; - let mut out: std::collections::HashMap = 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> { @@ -1083,12 +1047,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 writes_cells { + 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`. @@ -1119,11 +1085,13 @@ 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 delta = cell_delta(&self.model_state.workbook.model.data.drain_changes()); + let now = document_shape(&self.model_state.workbook); + self.model_state.dirty |= !delta.is_zero() || now != basis; + batch_delta += delta; + shape_basis = Some(now); } if matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry)) && let Some(basis) = config_basis @@ -1177,8 +1145,11 @@ impl App { break; } } - if let Some(before) = records_before { - self.bump_changed_records(&before); + if writes_cells { + 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); @@ -1206,20 +1177,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) { + /// 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 = 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); } } @@ -5231,6 +5193,71 @@ 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"); + } + + /// 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] diff --git a/crates/improvise-core/src/model/cell.rs b/crates/improvise-core/src/model/cell.rs index 2a913b2e..8e139a46 100644 --- a/crates/improvise-core/src/model/cell.rs +++ b/crates/improvise-core/src/model/cell.rs @@ -227,6 +227,7 @@ pub struct DataStore { /// The changes since `record_changes`, or `None` while nobody listens, so an /// unwatched store keeps no log. changes: Option>, + reorders: u64, } /// Resolved through the symbol table and sorted wherever the storage is a @@ -240,6 +241,7 @@ impl std::fmt::Debug for DataStore { records, arities: _, changes: _, + reorders: _, } = self; let cells: Vec<_> = cells .iter() @@ -290,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 = 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)> { @@ -851,6 +865,18 @@ mod data_store { 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();