From 28e9836759d72075632a9273abcb95ae49a4d8e9 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-backend)" Date: Sat, 10 Oct 2026 16:06:22 -0700 Subject: [PATCH] fix(incremental): a new aggregation reaches the stems that carry its measure; ordered evaluation re-runs the passes over a zero (improvise-shbc) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/improvise-incremental.md | 21 ++-- .../test-evidence/improvise-incremental.md | 27 ++++- crates/improvise-incremental/src/formulas.rs | 10 ++ .../improvise-incremental/src/incremental.rs | 25 ++++- .../improvise-incremental/tests/formulas.rs | 106 ++++++++++++++++++ .../tests/support/mod.rs | 28 +++-- 6 files changed, 191 insertions(+), 26 deletions(-) diff --git a/context/code/improvise-incremental.md b/context/code/improvise-incremental.md index f7a11ffd..af5dc684 100644 --- a/context/code/improvise-incremental.md +++ b/context/code/improvise-incremental.md @@ -269,17 +269,24 @@ is followed in outline, and the places it is not are the reason for this section formulas' dependency graph by name. When it has no cycle and there are fewer formulas than passes, one evaluation per cell in dependency order reaches the fixed point eager's passes reach (a chain of fifteen links needs fifteen passes and a sixteenth to see no change). It - differs from the passes in one place: a cell holding NaN is never equal to itself, so eager's passes - never see it settle and call it `circular`, and the cells that read it keep what they computed - from the NaN. `mark_nan_circular` does the same afterwards. `Evaluation::Plain` always runs - the passes; `tests/ordering.rs` compares the two and `tests/formulas.rs` compares both with - eager. + differs from the passes in two places. A cell holding NaN is never equal to itself, so eager's + passes never see it settle and call it `circular`, and the cells that read it keep what they + computed from the NaN; `mark_nan_circular` does the same afterwards. And eager stops on `==`, + which calls `-0.0` equal to the `0.0` an early pass read from a stored cell, so a stem with a + zero cell can settle on a sign dependency order never sees (a stored `0.0` at `F1`, `F3 = F1`, + `F4 = F3 ^ -1`, then `F1 = -0`: eager's `F4` is `inf`). A stem whose ordered evaluation holds any + zero is therefore evaluated again by the passes (`holds_a_zero`). `Evaluation::Plain` always + runs the passes; `tests/ordering.rs` compares the two after every edit, zero signs included, and + `tests/formulas.rs` compares both with eager. 6. **A formula edit rebuilds the stems.** Adding, removing or replacing a formula (the fingerprint of the formula list moves) rebuilds the stems from the store by one pass, clears the cache and evaluates every stem, because a formula has a cell at each. Clause 5 wants a formula delta; the index is not maintained across a formula edit. `Meter::rebuilds` counts it. -7. **A new aggregation dirties every stem if a formula reads that measure.** `measure_agg` is not - a cell, so no change reaches the readers; `reads_name` asks the formulas. +7. **A new aggregation dirties the stems it can reach.** `measure_agg` is not a cell, so no change + reaches the readers. A fold under a None category takes its aggregation from the `_Measure` + coordinate of the key, which for a formula targeting another category comes from the stem and + from no name the formula reads, so every live stem carrying that measure is dirtied + (`Stems::carrying`). Where a formula names the measure (it targets `_Measure`) `reads_name` dirties every stem. 8. **A write of the same bits changes nothing; a write of `-0.0` over `0.0` does.** The cell delta treats the two as one value (clause 3), but a formula may not (`Units ^ -1` is `inf` and `-inf`), so the formula side reads the raw change log, not the canonical delta. diff --git a/context/test-evidence/improvise-incremental.md b/context/test-evidence/improvise-incremental.md index 31c87228..e143ab2b 100644 --- a/context/test-evidence/improvise-incremental.md +++ b/context/test-evidence/improvise-incremental.md @@ -460,7 +460,6 @@ What each fault was and the test that names it: `a_cell_edit_evaluates_the_one_stem_that_reads_it_whatever_the_table_size`, `every_formula_in_the_pool_is_added_edited_around_and_removed_like_eager` and six others; - a stem that no cell gives keeps its cells: `a_new_record_adds_its_stem_and_a_cleared_one_takes_it_away`; -- reads skip the formulas a name targets: `formulas::tests::what_a_formula_reads_includes_what_the_formulas_it_names_read`; - reads skip the filters' categories: `a_filter_on_a_category_the_stem_has_reads_the_cells_of_other_stems`; - reads skip the formula's own target name: `a_new_aggregation_for_the_measure_a_filtered_formula_falls_back_to_changes_it`; @@ -488,7 +487,10 @@ What each fault was and the test that names it: the cache: `every_formula_in_the_pool_is_added_edited_around_and_removed_like_eager`, `incremental_matches_eager_on_a_pooled_total_of_a_ratio`. -**Faults that cannot be seen, and why.** A born stem is found again by the lookup of the cell that +**Faults that cannot be seen, and why.** Reads that skip the formulas a name targets change no answer +either (only the unit test `formulas::tests::what_a_formula_reads_includes_what_the_formulas_it_names_read` +fails): a stem is evaluated whole, so whatever the skipped formula would have added is +re-evaluated because the formula itself dirties the stem. A born stem is found again by the lookup of the cell that gave it, since the cell agrees with the stem outside the target category, so skipping the extra "born means dirty" step changes nothing. The target category being in a formula's reads cannot matter either: stems are derived by removing it, so no stem has it to drop. @@ -632,3 +634,24 @@ Both zeros in both orders. Fails on `Max` negative if any zero is. **Speed.** The corpus self-comparison and the maintained-against-fresh property moved to their own binaries (`tests/corpus_eager.rs`, `tests/maintained.rs`): `oracle.rs` ran 2.45 to 3.04 s in loaded full runs because its two heaviest tests shared it. The tests are unchanged. + +### Review of PR #351: aggregation edits, zero signs (improvise-shbc) + +### a_new_aggregation_reaches_the_stems_of_a_formula_that_targets_another_category + +`R0 = East + West` on Region, Time folded, `SetMeasureAgg Revenue Max`: eager reads 13 (the maximum of +East plus the maximum of West), the first version kept 14. Fails when a new aggregation dirties only +the stems of formulas that name the measure, and when `Stems::carrying` ignores the measure. + +### a_new_aggregation_reaches_a_product_targeted_filtered_sum + +The reviewer's second repro, a Product-targeted `SUM(F0 WHERE Region = East)`. It already agreed on this +fixture and stays as the second shape of the same hazard. + +### ordered_evaluation_keeps_the_zero_eager_settles_on + +A stored `0.0` at `F1`, then `F3 = F1`, `F4 = F3 ^ -1`, then `F1 = -0`: eager settles on `F3 = 0.0` and `F4 = inf` +because it calls `-0.0` equal to the `0.0` it first read; the first ordered evaluation gave `-inf`. Fails +(only `Ordered`) when a stem with a zero is not re-evaluated by the passes. The ordering property's pool +now holds `-0`, `Units * -1` and `-Cost`, and compares after every edit, but did not find this fault +alone in 1,500 cases: this test is the pin. diff --git a/crates/improvise-incremental/src/formulas.rs b/crates/improvise-incremental/src/formulas.rs index 78f100fd..f23d4147 100644 --- a/crates/improvise-incremental/src/formulas.rs +++ b/crates/improvise-incremental/src/formulas.rs @@ -3,6 +3,7 @@ use improvise_core::formula::ast::Expr; use improvise_core::model::Model; +use improvise_core::model::category::MEASURE_CATEGORY; use improvise_core::model::cell::{CellChange, CellKey}; use std::collections::{BTreeMap, BTreeSet, HashMap}; @@ -255,6 +256,15 @@ impl Stems { self.live.keys() } + /// The live stems whose own `_Measure` coordinate is `measure`: a fold under a None category + /// takes its aggregation from that coordinate, whether or not a formula names the measure. + pub fn carrying(&self, measure: &str) -> impl Iterator + '_ { + let measure = measure.to_string(); + self.live() + .filter(move |s| s.get(MEASURE_CATEGORY) == Some(measure.as_str())) + .cloned() + } + /// Whether any formula reads a cell under `name`, directly or through another formula. pub fn reads_name(&self, name: &str) -> bool { self.reads.iter().any(|r| r.names.contains(name)) diff --git a/crates/improvise-incremental/src/incremental.rs b/crates/improvise-incremental/src/incremental.rs index ede2e1f0..2ba5fff7 100644 --- a/crates/improvise-incremental/src/incremental.rs +++ b/crates/improvise-incremental/src/incremental.rs @@ -204,10 +204,11 @@ impl Incremental { self.clear_stem(stem); } let mut dirty = moved.dirty; - if let Edit::SetMeasureAgg { measure, .. } = edit - && self.stems.reads_name(measure) - { - dirty.extend(self.stems.live().cloned()); + if let Edit::SetMeasureAgg { measure, .. } = edit { + dirty.extend(self.stems.carrying(measure)); + if self.stems.reads_name(measure) { + dirty.extend(self.stems.live().cloned()); + } } self.evaluate(dirty); } @@ -253,7 +254,14 @@ impl Incremental { let value = self.evaluate_cell(&keys[i]); self.model.set_formula_cell(keys[i].clone(), value); } - self.mark_nan_circular(&keys); + if self.holds_a_zero(&keys) { + for key in &keys { + self.model.set_formula_cell(key.clone(), None); + } + self.passes(&keys); + } else { + self.mark_nan_circular(&keys); + } } _ => self.passes(&keys), } @@ -286,6 +294,13 @@ impl Incremental { } } + /// Eager stops on `==`, which calls `-0.0` equal to the `0.0` an early pass read, so a stem + /// with a zero cell can settle on a sign the dependency order never sees: it runs the passes. + fn holds_a_zero(&self, keys: &[CellKey]) -> bool { + let zero = |k: &CellKey| matches!(self.model.formula_cell(k), Some(CellValue::Number(n)) if *n == 0.0); + keys.iter().any(zero) + } + /// A NaN is not equal to itself, so eager's passes never see it settle and call it circular /// once they run out; the cells that read it keep what they computed from the NaN. fn mark_nan_circular(&mut self, keys: &[CellKey]) { diff --git a/crates/improvise-incremental/tests/formulas.rs b/crates/improvise-incremental/tests/formulas.rs index 2bc4883f..03cabdd2 100644 --- a/crates/improvise-incremental/tests/formulas.rs +++ b/crates/improvise-incremental/tests/formulas.rs @@ -292,3 +292,109 @@ fn a_new_record_adds_its_stem_and_a_cleared_one_takes_it_away() { ]; same_model(&wb.model, &none, &edits, Evaluation::Ordered).unwrap(); } + +/// Region {East, West}, a Time category folded away, Revenue by quarter: `R0 = East + West` +/// targets Region, so its stems carry the measure but no formula names it. +fn region_target_model() -> Model { + let mut model = Model::new("regions"); + for (cat, items) in [("Region", vec!["East", "West"]), ("Time", vec!["Q1", "Q2"])] { + model.add_category(cat).unwrap(); + for item in items { + model.category_mut(cat).unwrap().add_item(item); + } + } + model.category_mut("_Measure").unwrap().add_item("Revenue"); + for (region, quarter, v) in [ + ("East", "Q1", 1.0), + ("East", "Q2", 3.0), + ("West", "Q1", 10.0), + ] { + let key = CellKey::new(vec![ + ("Region".into(), region.into()), + ("Time".into(), quarter.into()), + ("_Measure".into(), "Revenue".into()), + ]); + model.set_cell(key, CellValue::Number(v)); + } + model.add_formula(parse_formula("R0 = East + West", "Region").unwrap()); + model +} + +#[test] +fn a_new_aggregation_reaches_the_stems_of_a_formula_that_targets_another_category() { + let model = region_target_model(); + let none = vec!["Time".to_string()]; + let edits = [Edit::SetMeasureAgg { + measure: "Revenue".into(), + func: improvise_core::formula::ast::AggFunc::Max, + }]; + for by in [Evaluation::Ordered, Evaluation::Plain] { + same_model(&model, &none, &edits, by).unwrap(); + } + let mut backend = Incremental::new(model, none); + backend.apply(&edits[0]); + let at = CellKey::new(vec![ + ("Region".into(), "R0".into()), + ("_Measure".into(), "Revenue".into()), + ]); + let want = Some(CellValue::Number(13.0)); + assert_eq!( + backend.value(&at, Aggregation::OverHidden), + want, + "Max 3 + Max 10" + ); +} + +#[test] +fn a_new_aggregation_reaches_a_product_targeted_filtered_sum() { + let mut model = Model::new("products"); + for (cat, items) in [("Region", vec!["East", "West"]), ("Product", vec!["F0"])] { + model.add_category(cat).unwrap(); + for item in items { + model.category_mut(cat).unwrap().add_item(item); + } + } + model.category_mut("_Measure").unwrap().add_item("Cost"); + for (region, v) in [("East", 4.0), ("West", 9.0)] { + let key = CellKey::new(vec![ + ("Region".into(), region.into()), + ("Product".into(), "F0".into()), + ("_Measure".into(), "Cost".into()), + ]); + model.set_cell(key, CellValue::Number(v)); + } + model.add_formula(parse_formula("P0 = SUM(F0 WHERE Region = East)", "Product").unwrap()); + let none = vec![]; + let edits = [Edit::SetMeasureAgg { + measure: "Cost".into(), + func: improvise_core::formula::ast::AggFunc::Min, + }]; + for by in [Evaluation::Ordered, Evaluation::Plain] { + same_model(&model, &none, &edits, by).unwrap(); + } +} + +/// A stored `0.0` at `F1`, then `F3 = F1`, `F4 = F3 ^ -1` and last `F1 = -0`: eager's first +/// pass reads the stored zero, and its later passes call `-0.0` equal to it and stop. +#[test] +fn ordered_evaluation_keeps_the_zero_eager_settles_on() { + let mut model = Model::new("zeros"); + model.add_category("Region").unwrap(); + model.category_mut("Region").unwrap().add_item("East"); + let key = CellKey::new(vec![ + ("Region".into(), "East".into()), + ("_Measure".into(), "F1".into()), + ]); + model.set_cell(key, CellValue::Number(0.0)); + let edits: Vec = [ + ("F3 = F1", "_Measure"), + ("F4 = F3 ^ -1", "_Measure"), + ("F1 = -0", "_Measure"), + ] + .iter() + .map(|(raw, cat)| Edit::AddFormula(parse_formula(raw, cat).unwrap())) + .collect(); + for by in [Evaluation::Ordered, Evaluation::Plain] { + same_model(&model, &[], &edits, by).unwrap_or_else(|e| panic!("{by:?}: {e}")); + } +} diff --git a/crates/improvise-incremental/tests/support/mod.rs b/crates/improvise-incremental/tests/support/mod.rs index d4f6f45c..edc5a71c 100644 --- a/crates/improvise-incremental/tests/support/mod.rs +++ b/crates/improvise-incremental/tests/support/mod.rs @@ -238,7 +238,7 @@ pub fn hidden_counted_workbook() -> improvise_core::workbook::Workbook { /// Formulas the random tests add and remove, as `(text, target category)`: chains, a cycle, /// aggregates over expressions, filters, a ratio, and two that target Region. -pub const FORMULA_POOL: [(&str, &str); 18] = [ +pub const FORMULA_POOL: [(&str, &str); 21] = [ ("Profit = Revenue - Cost", "_Measure"), ("Margin = Profit / Revenue", "_Measure"), ("Weighted = SUM(Revenue * Units)", "_Measure"), @@ -257,6 +257,9 @@ pub const FORMULA_POOL: [(&str, &str); 18] = [ ("EastRev = Revenue", "Region"), ("Recip = Units ^ -1", "_Measure"), ("Pa = A + Revenue", "_Measure"), + ("NegZero = -0", "_Measure"), + ("Flip = Units * -1", "_Measure"), + ("Negate = -Cost", "_Measure"), ]; pub fn formula_edit(index: usize) -> Edit { @@ -371,23 +374,24 @@ pub fn any_formula_edit() -> impl Strategy { ] } -/// The same edits told to a backend that evaluates in dependency order and to one that always -/// runs eager's passes: their models, formula cache included, must print alike. +/// The same edits told to a backend that evaluates in dependency order and to one that runs +/// eager's passes: models, cache and zero signs must print alike after every edit. pub fn same_orders(model: &Model, none: &[String], edits: &[Edit]) -> Result<(), String> { let side = |by| Incremental::new(model.clone(), none.to_vec()).with_evaluation(by); let (mut ordered, mut plain) = (side(Evaluation::Ordered), side(Evaluation::Plain)); - for edit in edits { + for (n, edit) in edits.iter().enumerate() { ordered.apply(edit); plain.apply(edit); + let (want, got) = ( + format!("{:?}", plain.model()), + format!("{:?}", ordered.model()), + ); + if want != got { + let told = &edits[..=n]; + return Err(format!("after {told:?}:\n plain {want}\n ordered {got}")); + } } - let (want, got) = ( - format!("{:?}", plain.model()), - format!("{:?}", ordered.model()), - ); - match want == got { - true => Ok(()), - false => Err(format!("after {edits:?}:\n plain {want}\n ordered {got}")), - } + Ok(()) } /// An `Eager` rebuilt from scratch on every edit: maintaining one in place