Merge dbsp/live-source into dbsp/pushdown

context/code/source.md

@@ -70,14 +70,15 @@ cells it never owns.
 1. **A model over a source is read-only, in every build.** `set_cell`, `clear_cell`, `remove_category` and
    `remove_item` `assert!` that no source is set (a `debug_assert` first shipped and was lost in release:
    the write went to the owned store, which nothing reads). A caller who holds a source writes to the
-   source (ADR 003: the trait has no writes). `Model::over` drops the model's own cells, so no stale copy
+   source (ADR 003: the trait has no writes). `Model::over` drops the model's own cells and the formula cells computed from them, so no stale copy
    is left for `get_cell`, persistence or the harness to read. `data` is `pub`, so a caller can still write
    it directly; it is not read, and nothing prevents that. Structural edits (a category or item removed)
    are refused too, because the source's cells would still carry them.
 2. **A shared source changes behind its readers by interior mutability of its own.** The model holds
    an `Arc`, so a database connection or a test double behind a lock is the same object the
    backend feeds from.
-3. **What still reads the owned store**: `get_cell`, persistence, the import wizard and the effects
+3. **What still reads the owned store**: `get_cell` (a production caller: `improvise-command/src/ui/effect.rs`,
+   the `old` value of a rename), `Model::data` (`pub`), persistence, the import wizard and the effects
    in `improvise-command`. They are the facade of a document that owns its cells; a model over a source
    has none of those to offer, and routing them would change `get_cell`'s `&CellValue` return for 129
    call sites. `Eager::occupancy`, `Shapes::of` and `Stems::build` in `improvise-incremental` are
@@ -123,6 +124,8 @@ backend a Revenue of 128 and the other 38, and the second then disagreed with it
 
 ## Building a `Summary` from an engine's answer
 
-`Summary::from_parts(Numbers::from_parts(count, exact_sum, least, greatest), Agreement)`. `Agreement`
-is what the non-number values hold: `None`, `One(v)` or `Mixed`. `from_parts` drops extremes that
-contradict the count or are NaN, so a source cannot hand the evaluator a state `add` could not reach.
+`Summary::from_parts(Numbers::from_parts(count, exact_sum, least, greatest)?, Agreement)?`. `Agreement`
+is what the non-number values hold: `None`, `One(v)` or `Mixed`. Both constructors return a `Result`
+and **refuse** a state no sequence of values could produce (extremes with no numbers, a NaN extreme,
+least above greatest, one extreme without the other, a number among the "others"), because repairing
+it would hide an engine's bug.

context/test-evidence/core.md

@@ -2314,6 +2314,11 @@ lost). The first two also fail `aggregating_differing_text_values_is_an_explicit
 
 ### model::source::tests::a_merge_of_summaries_is_the_summary_of_the_union
 
+Review of #356 (Opus): the pool gained `1e16`, `1.0`, `-1e16`, `0.1` and `0.2`. A merge rounding each part's
+sum before pooling, keeping the zero sign, changed 514 production answers while the workspace passed;
+that rival now fails this test, `numbers_pool_what_the_formula_aggregate_pools` (a NaN-free split now,
+compared as bits), `summaries_built_from_parts_merge_as_the_union_does` and the fixture test below, by name.
+
 A read of several parts is a merge. The test merges every ordered pair of sequences of up to two
 values and compares with the reference of the concatenation. Rivals: the least not merged, the
 greatest merged with the other side's least, the zeros unordered. Each fails it.
@@ -2335,7 +2340,7 @@ span, and the store ignores `exact` for a spanned one. The first also fails
 ### model::source::tests::the_store_and_a_scanned_table_list_the_same_cells
 
 `keys` and `cells` under every base of the fixture, and `value_at` for every row and an absent key.
-Rival: `DataStore::keys` ignores its partial. It fails here and in five evaluator tests that read
+Rival: `DataStore::keys` ignores its partial. It fails here and in seven evaluator tests that read
 the member records of an aggregate over an expression
 (`route_then_aggregate_tests::a_routing_formula_at_a_record_is_the_rule_itself` among them).
 
@@ -2405,13 +2410,28 @@ fails several evaluator tests).
 ### a_summary_built_from_an_engines_parts_reduces_as_one_built_from_the_values
 
 Review of #356 (Codex): an engine outside core could not build a `Summary` without replaying rows. The
-parts (count, exact sum, extremes, agreement of the non-numbers) are computed in the test without
-`Summary::of`, for the 585 sequences of the reference test. Rivals: the greatest taken from the least;
-a NaN allowed as an extreme. Each fails it.
+parts are computed in the test without `Summary::of`, for every sequence of up to three values of the
+pool. No rival of the first version failed this test alone (its filter was untested); see below.
 
-### parts_that_contradict_themselves_are_made_consistent
+### summaries_built_from_parts_merge_as_the_union_does
 
-Extremes with no numbers, and a NaN as an extreme, are dropped by `Numbers::from_parts`.
+A summary built from parts, merged with one built from values and with one built from parts, equals the
+reference of the union, for every pair of sequences of up to two values. This is where a bad extreme
+would matter, since merge reads the extremes.
+
+### parts_no_sequence_of_values_could_produce_are_refused
+
+Review of #356 (Opus, Codex): `from_parts` repaired some impossible states and accepted others
+(`One(Number)`, least above greatest). It refuses all of them with a `BadParts` naming the reason.
+The earlier entry's claim that a NaN-extreme rival failed `parts_that_contradict_themselves_...` was
+false: that test passed with the filter removed. Rivals now, each failing this test alone: NaN extreme
+allowed, least above greatest allowed, extremes with no numbers allowed, a number accepted among the
+others.
+
+### the_exact_sum_of_a_pool_of_cancelling_values_is_not_the_sum_of_rounded_parts
+
+An exactness test needs inputs whose float sum is inexact and no NaN. `1e16 + 1.0 - 1e16` rounded part
+by part is `0.0`; pooled exactly it is `1.0`. This pins the fixture rather than the code.
 
 ### keys_of_a_store_are_its_cells_keys_without_the_values
 
@@ -2443,3 +2463,11 @@ source holds, and the evaluator still reads the source's value.
 
 Review of #357: a records-mode `GridLayout` over a source. `group_records` reading the owned store
 left all 563 tests green; with this test it fails (the grid has no rows).
+
+### putting_a_model_with_formula_cells_over_a_source_drops_what_those_cells_computed
+
+Review of #357 (Codex): `Model::over` emptied the store and kept `formula_cache`, which was computed from
+those cells; reads consult the cache first, so old formula values survived until a recompute. The model
+has a computed Profit, the source holds a different revenue, and the cell must be gone after `over`.
+Rival: the `formula_cache.clear()` removed fails this test alone (before the fix it failed here first).
+`measure_agg` and the categories are the definition and stay; nothing else is derived from the cells.

crates/improvise-core/src/model/over_tests.rs

@@ -286,3 +286,37 @@ fn the_records_a_drill_freezes_are_read_from_the_source() {
     }
     assert!(rows > 10, "only {rows} records were compared");
 }
+
+#[test]
+fn putting_a_model_with_formula_cells_over_a_source_drops_what_those_cells_computed() {
+    let mut owned = fixture();
+    owned.recompute_formulas(&["Region".to_string()]);
+    let profit = CellKey::new(vec![
+        ("Product".into(), "A".into()),
+        ("_Measure".into(), "Profit".into()),
+    ]);
+    let stale = owned.evaluate_aggregated(&profit, Aggregation::OverHidden);
+    assert!(
+        matches!(stale, Some(CellValue::Number(_))),
+        "the cache holds Profit"
+    );
+    let revenue = key("East", "A", "Revenue");
+    let mut rows: Vec<_> = owned
+        .data
+        .iter_cells()
+        .map(|(k, v)| (k, v.clone()))
+        .collect();
+    rows.iter_mut()
+        .filter(|(k, _)| *k == revenue)
+        .for_each(|(_, v)| *v = CellValue::Number(1000.0));
+    let over = owned.clone().over(Arc::new(TableSource { rows }));
+    assert_eq!(
+        over.formula_cell(&profit),
+        None,
+        "computed from cells that are gone"
+    );
+    assert_ne!(
+        over.evaluate_aggregated(&profit, Aggregation::OverHidden),
+        stale
+    );
+}

crates/improvise-core/src/model/source.rs

@@ -62,20 +62,28 @@ impl Numbers {
     }
 
     /// Numbers from what an engine computed: how many, their exact sum and the least and
-    /// greatest that are not NaN. Extremes that contradict the count or are NaN are dropped.
+    /// greatest that are not NaN. A state `add` cannot reach is refused, not repaired.
     pub fn from_parts(
         count: u64,
         sum: ExactSum,
         least: Option<f64>,
         greatest: Option<f64>,
-    ) -> Self {
-        let held = |x: Option<f64>| x.filter(|x| count > 0 && !x.is_nan());
-        Numbers {
+    ) -> Result<Self, BadParts> {
+        let bad = |why| Err(BadParts(why));
+        match (least, greatest) {
+            (None, None) => {}
+            (Some(_), Some(_)) if count == 0 => return bad("extremes with no numbers"),
+            (Some(l), Some(g)) if l.is_nan() || g.is_nan() => return bad("a NaN extreme"),
+            (Some(l), Some(g)) if l.total_cmp(&g).is_gt() => return bad("least above greatest"),
+            (Some(_), Some(_)) => {}
+            _ => return bad("one extreme without the other"),
+        }
+        Ok(Numbers {
             count,
             sum,
-            least: held(least),
-            greatest: held(greatest),
-        }
+            least,
+            greatest,
+        })
     }
 
     pub fn add(&mut self, x: f64) {
@@ -131,6 +139,10 @@ fn merged(a: Option<f64>, b: Option<f64>, pick: fn(f64, f64) -> f64) -> Option<f
     }
 }
 
+/// Parts of a summary that no sequence of values could have produced.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct BadParts(pub &'static str);
+
 /// What the values that are not numbers agree on: nothing held, one value, or disagreement.
 #[derive(Debug, Clone, Default, PartialEq)]
 pub enum Agreement {
@@ -156,8 +168,11 @@ impl Summary {
     }
 
     /// A summary from what an engine computed: the numbers, and what the rest agree on.
-    pub fn from_parts(numbers: Numbers, others: Agreement) -> Self {
-        Summary { numbers, others }
+    pub fn from_parts(numbers: Numbers, others: Agreement) -> Result<Self, BadParts> {
+        match &others {
+            Agreement::One(v) if v.as_f64().is_some() => Err(BadParts("a number among the others")),
+            _ => Ok(Summary { numbers, others }),
+        }
     }
 
     pub fn add(&mut self, value: &CellValue) {
@@ -397,6 +412,11 @@ mod tests {
             CellValue::Number(0.0),
             CellValue::Number(f64::NAN),
             CellValue::Number(f64::INFINITY),
+            CellValue::Number(1e16),
+            CellValue::Number(1.0),
+            CellValue::Number(-1e16),
+            CellValue::Number(0.1),
+            CellValue::Number(0.2),
             text("a"),
             text("b"),
             CellValue::Error("e".into()),
@@ -489,23 +509,41 @@ mod tests {
 
     #[test]
     fn numbers_pool_what_the_formula_aggregate_pools() {
-        let xs = [1.0, -0.0, f64::NAN, 3.0, 0.0];
+        let xs = [1e16, 1.0, -1e16, 0.1, 0.2, -0.0, 0.0, 3.0];
         for split in 0..=xs.len() {
             let mut pooled = Numbers::of(xs[..split].iter().copied());
             pooled.merge(&Numbers::of(xs[split..].iter().copied()));
             let whole = Numbers::of(xs);
             for func in &FUNCS {
                 let (a, b) = (pooled.reduce(func), whole.reduce(func));
-                let equal = a.map(f64::to_bits) == b.map(f64::to_bits)
-                    || a.zip(b).is_some_and(|(x, y)| x.is_nan() && y.is_nan());
-                assert!(equal, "{func:?} split at {split}: {a:?} != {b:?}");
+                assert_eq!(
+                    a.map(f64::to_bits),
+                    b.map(f64::to_bits),
+                    "{func:?} split at {split}"
+                );
             }
-            assert_eq!(pooled.total().to_bits(), whole.total().to_bits());
+            assert_eq!(
+                pooled.total().to_bits(),
+                whole.total().to_bits(),
+                "split at {split}"
+            );
         }
         assert_eq!(Numbers::default().total().to_bits(), 0.0f64.to_bits());
         assert_eq!(Numbers::default().reduce(&AggFunc::Sum), None);
     }
 
+    #[test]
+    fn the_exact_sum_of_a_pool_of_cancelling_values_is_not_the_sum_of_rounded_parts() {
+        let rounded = [1e16, 1.0].iter().sum::<f64>() + [-1e16].iter().sum::<f64>();
+        let mut pooled = Numbers::of([1e16, 1.0]);
+        pooled.merge(&Numbers::of([-1e16]));
+        assert_eq!(
+            rounded, 0.0,
+            "the fixture is inexact: summing rounded parts loses the 1"
+        );
+        assert_eq!(pooled.total(), 1.0);
+    }
+
     fn pair(c: &str, i: &str) -> (String, String) {
         (c.to_string(), i.to_string())
     }
@@ -750,8 +788,8 @@ mod tests {
             }
             Some(_) => Agreement::Mixed,
         };
-        let parts = Numbers::from_parts(numbers.len() as u64, sum, least, greatest);
-        Summary::from_parts(parts, agreement)
+        let parts = Numbers::from_parts(numbers.len() as u64, sum, least, greatest).unwrap();
+        Summary::from_parts(parts, agreement).unwrap()
     }
 
     #[test]
@@ -770,12 +808,61 @@ mod tests {
     }
 
     #[test]
-    fn parts_that_contradict_themselves_are_made_consistent() {
-        let none = Numbers::from_parts(0, ExactSum::new(), Some(1.0), Some(f64::NAN));
-        assert_eq!(none.reduce(&AggFunc::Min), None, "no numbers, no extremes");
-        let nan = Numbers::from_parts(2, ExactSum::new(), Some(f64::NAN), None);
-        let min = nan.reduce(&AggFunc::Min).unwrap();
-        assert!(min.is_nan(), "a NaN is never an extreme");
+    fn summaries_built_from_parts_merge_as_the_union_does() {
+        let all = sequences(2);
+        for (a, b) in all.iter().flat_map(|a| all.iter().map(move |b| (a, b))) {
+            let mut merged = parts_of(a);
+            merged.merge(&Summary::of(b));
+            let mut both = parts_of(a);
+            both.merge(&parts_of(b));
+            let union: Vec<&CellValue> = a.iter().chain(b).collect();
+            for func in &FUNCS {
+                let want = reference(&union, func);
+                assert!(
+                    same(&want, &merged.reduce(func)),
+                    "{func:?} of {a:?} + {b:?}"
+                );
+                assert!(
+                    same(&want, &both.reduce(func)),
+                    "{func:?} of {a:?} + {b:?}, both from parts"
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn parts_no_sequence_of_values_could_produce_are_refused() {
+        let sum = ExactSum::new;
+        let refused = |r: Result<Numbers, BadParts>| r.expect_err("must be refused").0;
+        assert_eq!(
+            refused(Numbers::from_parts(0, sum(), Some(1.0), Some(1.0))),
+            "extremes with no numbers"
+        );
+        assert_eq!(
+            refused(Numbers::from_parts(2, sum(), Some(f64::NAN), Some(1.0))),
+            "a NaN extreme"
+        );
+        assert_eq!(
+            refused(Numbers::from_parts(2, sum(), Some(2.0), Some(1.0))),
+            "least above greatest"
+        );
+        assert_eq!(
+            refused(Numbers::from_parts(2, sum(), Some(0.0), Some(-0.0))),
+            "least above greatest"
+        );
+        assert_eq!(
+            refused(Numbers::from_parts(2, sum(), Some(1.0), None)),
+            "one extreme without the other"
+        );
+        let ok = Numbers::from_parts(2, sum(), None, None).unwrap();
+        assert!(
+            ok.reduce(&AggFunc::Min).unwrap().is_nan(),
+            "all NaN: no extremes, a count"
+        );
+        let number = Agreement::One(CellValue::Number(1.0));
+        let bad = Summary::from_parts(ok.clone(), number).expect_err("a number is not an other");
+        assert_eq!(bad.0, "a number among the others");
+        assert!(Summary::from_parts(ok, Agreement::One(CellValue::Text("t".into()))).is_ok());
     }
 
     #[test]

crates/improvise-core/src/model/types.rs

@@ -431,9 +431,10 @@ impl Model {
     }
 
     /// This model, reading its raw cells from `source` and not `data`: read-only. Its own
-    /// cells are dropped, so no stale second copy is left to read. Why: `context/code/source.md`.
+    /// cells are dropped, and the formula cells computed from them, so no stale copy is left to read. Why: `context/code/source.md`.
     pub fn over(mut self, source: SharedSource) -> Self {
         self.data = DataStore::new();
+        self.formula_cache.clear();
         self.external = Some(source);
         self
     }