diff --git a/Cargo.lock b/Cargo.lock index 97206bb9..5145d3c7 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/Cargo.nix b/Cargo.nix index 4660a79b..17373cea 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -2903,6 +2903,10 @@ rec { name = "improvise-gp"; packageId = "improvise-gp"; } + { + name = "improvise-incremental"; + packageId = "improvise-incremental"; + } { name = "improvise-io"; packageId = "improvise-io"; @@ -3069,6 +3073,10 @@ rec { name = "improvise-core"; packageId = "improvise-core"; } + { + name = "zset"; + packageId = "zset"; + } ]; devDependencies = [ { diff --git a/context/code/command.md b/context/code/command.md index 843d0f0d..8caa5155 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -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` diff --git a/context/code/improvise-incremental.md b/context/code/improvise-incremental.md index 35e39331..24469197 100644 --- a/context/code/improvise-incremental.md +++ b/context/code/improvise-incremental.md @@ -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` 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 diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 08ce94cb..fe594059 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -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. diff --git a/context/test-evidence/improvise-incremental.md b/context/test-evidence/improvise-incremental.md index aac72cbc..a96c7cb7 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 87f02fd2..bc80a6b1 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> { @@ -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) { + /// 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); } } @@ -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, + ], + 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] diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index f69d2ff8..def5979e 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -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 { + 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] diff --git a/crates/improvise-core/src/model/cell.rs b/crates/improvise-core/src/model/cell.rs index ab23eb70..8e139a46 100644 --- a/crates/improvise-core/src/model/cell.rs +++ b/crates/improvise-core/src/model/cell.rs @@ -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, + pub new: Option, +} + #[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, + /// 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 @@ -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 = 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 { + 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, new: Option| 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(); diff --git a/crates/improvise-incremental/Cargo.toml b/crates/improvise-incremental/Cargo.toml index 3da249b2..2eed040a 100644 --- a/crates/improvise-incremental/Cargo.toml +++ b/crates/improvise-incremental/Cargo.toml @@ -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" } diff --git a/crates/improvise-incremental/src/delta.rs b/crates/improvise-incremental/src/delta.rs new file mode 100644 index 00000000..a14c565e --- /dev/null +++ b/crates/improvise-incremental/src/delta.rs @@ -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 { + 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) -> 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 { + let mut records: Vec = 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, new: Option) -> CellChange { + CellChange { + key: k.clone(), + old, + new, + } + } + + fn num(n: f64) -> CellValue { + CellValue::Number(n) + } + + fn delta_of(old: Option, new: Option) -> 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 = 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 = + [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 = 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"); + } +} diff --git a/crates/improvise-incremental/src/lib.rs b/crates/improvise-incremental/src/lib.rs index 0f006f9b..69f4a18a 100644 --- a/crates/improvise-incremental/src/lib.rs +++ b/crates/improvise-incremental/src/lib.rs @@ -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::{ diff --git a/crates/improvise-incremental/tests/oracle.rs b/crates/improvise-incremental/tests/oracle.rs index 6b3304be..9fa50334 100644 --- a/crates/improvise-incremental/tests/oracle.rs +++ b/crates/improvise-incremental/tests/oracle.rs @@ -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 { ] } +/// 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 { + 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 { + 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 { prop_oneof![ (any_key(), any_value()).prop_map(|(k, v)| Edit::SetCell(k, v)), diff --git a/nix/ci.nix b/nix/ci.nix index 1b0500f3..fa189273 100644 --- a/nix/ci.nix +++ b/nix/ci.nix @@ -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"