From f7f7769460e6589a062aa4c930fa4b8db4534a2b Mon Sep 17 00:00:00 2001 From: Sasha Syrotenko Date: Sun, 11 Oct 2026 17:50:23 +0300 Subject: [PATCH] Fold 'SUM', 'COUNT' and 'AVERAGE' over ranges in place --- crates/calc/src/eval.rs | 123 ++++++++++++++++++++++++++ crates/calc/src/recalc.rs | 65 +++++++++++++- crates/calc/src/tests.rs | 182 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 368 insertions(+), 2 deletions(-) diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index eff659d..bf5c971 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -71,6 +71,11 @@ pub trait Host { fn used_range(&mut self, sheet: usize) -> Option { self.workbook().sheet(sheet).and_then(|s| s.used_range()) } + /// Calls `f` on each stored value of a block, row-major (formula cells evaluated first), + /// until `f` returns false. `false` when this host can't (callers use `range_values`). + fn visit_range(&mut self, _sheet: usize, _range: RangeRef, _f: &mut dyn FnMut(&Value) -> bool) -> bool { + false + } } pub struct Evaluator<'h> { @@ -726,6 +731,9 @@ impl<'h> Evaluator<'h> { if args.len() < spec.min_args || args.len() > spec.max_args { return err(CellError::Value); } + if let Some(kind) = Agg::of(spec.name) { + return self.aggregate(kind, args); + } let mut evaluated = Vec::with_capacity(args.len()); // SUMIF/AVERAGEIF: the size of the criteria range (argument 1), which the sum range takes. let mut criteria_shape = None; @@ -762,6 +770,70 @@ impl<'h> Evaluator<'h> { gridcraft_functions::call(spec, &evaluated, &mut ctx).pipe(Ev::V) } + /// Performs aggregation operations without copying its ranges: blocks are folded over the stored + /// cells, so no array is built and there is no `MAX_CELLS` limit. Arguments are evaluated and read one + /// after another, as the array path does, so volatile formulas (like `RAND`) see the same order. + /// Only `SUM`, `COUNT` and `AVG` are supported currently. + fn aggregate(&mut self, kind: Agg, args: &[Expr]) -> Ev { + let mut acc = Acc::default(); + let mut first_error = None; + for a in args { + let ev = self.eval(a); + let steps = match ev { + Ev::R(areas) => areas.iter().map(|area| self.aggregate_area(kind, area, &mut acc)).collect(), + ev => { + let value = self.deref(ev); + vec![self.aggregate_value(kind, value, false, &mut acc)] + } + }; + if first_error.is_none() { + first_error = steps.into_iter().find_map(Result::err); + } + } + first_error.unwrap_or_else(|| Ev::V(acc.finish(kind))) + } + + /// Adds one area to `acc`; `Err` is the result the function ends with. + fn aggregate_area(&mut self, kind: Agg, a: &Area, acc: &mut Acc) -> Result<(), Ev> { + if !a.range.is_single() { + let mut error = None; + let walked = self.host.visit_range(a.sheet, a.range, &mut |v| match acc.add_cell(kind, v) { + Ok(()) => true, + Err(e) => { + error = Some(e); + false + } + }); + if walked { + return error.map_or(Ok(()), |e| Err(err(e))); + } + } + let value = self.area_value(a); + self.aggregate_value(kind, value, true, acc) + } + + /// Adds one argument's value by calling `SUM` and `COUNT` on it, so coercion and error rules + /// stay in one place (`AVERAGE` of an argument is its `SUM` over its `COUNT`). + fn aggregate_value(&mut self, kind: Agg, value: Value, from_ref: bool, acc: &mut Acc) -> Result<(), Ev> { + let (Some(sum), Some(count)) = (gridcraft_functions::lookup("SUM"), gridcraft_functions::lookup("COUNT")) else { + return Err(err(CellError::Name)); + }; + let args = [Arg { value, from_ref }]; + if kind != Agg::Count { + match gridcraft_functions::call(sum, &args, &mut FnCtx { ev: self }) { + Value::Number(n) => acc.sum += n, + other => return Err(Ev::V(other)), + } + } + if kind != Agg::Sum { + match gridcraft_functions::call(count, &args, &mut FnCtx { ev: self }) { + Value::Number(n) => acc.count = acc.count.saturating_add(n as u64), + other => return Err(Ev::V(other)), + } + } + Ok(()) + } + pub fn apply_lambda(&mut self, l: &Lambda, args: &[Expr]) -> Ev { if args.len() > l.params.len() { return err(CellError::Value); @@ -1239,6 +1311,57 @@ pub fn r1c1_to_a1(s: &str, at: CellRef) -> Option { Some(format!("{sheet}{out}")) } +/// The aggregates that can fold a reference without building an array. +#[derive(Clone, Copy, PartialEq, Eq)] +enum Agg { + Sum, + Count, + Average, +} + +impl Agg { + fn of(name: &str) -> Option { + match name { + "SUM" => Some(Agg::Sum), + "COUNT" => Some(Agg::Count), + "AVERAGE" => Some(Agg::Average), + _ => None, + } + } +} + +/// What a folded aggregate carries from one value to the next. +#[derive(Default)] +struct Acc { + sum: f64, + count: u64, +} + +impl Acc { + /// Takes a value found inside a reference: only numbers count. `Err` is an error value that + /// ends `SUM` and `AVERAGE`; `COUNT` skips errors. + fn add_cell(&mut self, kind: Agg, v: &Value) -> Result<(), CellError> { + match v { + Value::Number(n) => { + self.sum += n; + self.count = self.count.saturating_add(1); + Ok(()) + } + Value::Error(e) if kind != Agg::Count => Err(*e), + _ => Ok(()), + } + } + + fn finish(&self, kind: Agg) -> Value { + match kind { + Agg::Sum => Value::number(self.sum), + Agg::Count => Value::number(self.count as f64), + Agg::Average if self.count == 0 => Value::Error(CellError::Div0), + Agg::Average => Value::number(self.sum / self.count as f64), + } + } +} + // ---------------------------------------------------------------- operators fn map_unary(v: &Value, f: &dyn Fn(f64) -> Value, plus: bool) -> Value { diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index 4cf0c6a..74c406b 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -598,6 +598,30 @@ impl Host for PassHost<'_> { } out } + fn visit_range(&mut self, sheet: usize, range: RangeRef, f: &mut dyn FnMut(&Value) -> bool) -> bool { + let wb = self.wb; + let Some(sh) = wb.sheet(sheet) else { return false }; + // Formula cells first, in row-major order like `range_values`: reading one calculates it + // and the pass remembers the value, so the walk below gets the same value again without + // this method holding any. + for (c, cell) in sh.cells.iter_range(range) { + if cell.formula.is_some() { + let _ = self.cell_value(sheet, c); + } + } + // Spilled values sit between the stored cells (and the formulas just calculated may have + // produced some): leave such blocks to `range_values`. + if self.spills.keys().any(|a| a.0 == sheet) || sh.spill.range(range.start..=range.end).any(|(c, _)| range.contains(*c)) { + return false; + } + for (c, cell) in sh.cells.iter_range(range) { + let go = if cell.formula.is_some() { f(&self.cell_value(sheet, c)) } else { f(&cell.value) }; + if !go { + break; + } + } + true + } fn used_range(&mut self, sheet: usize) -> Option { let wb = self.wb; *self.used.entry(sheet).or_insert_with(|| wb.sheet(sheet).and_then(|s| s.used_range())) @@ -974,13 +998,13 @@ fn prof_now() -> f64 { } #[cfg(test)] -mod tests { +pub(crate) mod tests { use std::time::{Duration, Instant}; use gridcraft_core::{CellRef, RangeRef, Value}; use gridcraft_model::{Cell, Formula, Workbook}; - use super::{Calc, Intervals, Key}; + use super::{Calc, HashMap, HashSet, Host, Intervals, Key, PassHost}; /// xorshift64 for repeatable pseudo-random cases. fn rng(state: &mut u64) -> u64 { @@ -1181,4 +1205,41 @@ mod tests { calc.graph.dependents(1, CellRef::parse("A2").unwrap(), &mut deps); assert_eq!(deps, vec![at("DA9"), at("DA9")]); } + + /// For tests of `PassHost::visit_range`, which can't be reached from outside this module: the + /// values it hands over for `range` while the formulas in `pending` (cell, formula) have not been + /// calculated yet, or `None` when it declines the block. + pub(crate) fn probe_visit_range(wb: &Workbook, pending: &[(&str, &str)], range: RangeRef) -> Option> { + let mut exprs = HashMap::default(); + for (at, formula) in pending { + let expr = gridcraft_formula::parse(formula.strip_prefix('=').unwrap_or(formula)).ok()?; + exprs.insert((0, CellRef::parse(at)?), std::sync::Arc::new(expr)); + } + + // A cell in `pending` is calculated the first time something reads it, which is the state + // of a real pass partway through; `visit_range` has to cope with the formulas it reads + // being calculated under it. + let mut rng = 1u64; + let mut host = PassHost { + wb, + pending: exprs.keys().copied().collect(), + exprs: &exprs, + results: HashMap::default(), + in_progress: HashSet::default(), + spills: HashMap::default(), + used: HashMap::default(), + rng: &mut rng, + now: 0.0, + cycle: false, + cycles: vec![], + depth: 0, + }; + + let mut seen = Vec::new(); + host.visit_range(0, range, &mut |v| { + seen.push(v.clone()); + true + }) + .then_some(seen) + } } diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index d1e8576..5fb6410 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -467,3 +467,185 @@ fn sumif_resized_sum_range_on_another_sheet_and_through_a_name() { assert_eq!(t.num("C1"), 10.0); assert_eq!(t.num("C2"), 5.0); } + +/// `formula` in AB1 over a small mixed sheet. The ranges in the cases below are cut from it: +/// +/// ```text +/// A B C E +/// 1 1.5 10 =A1*4 (C1 is a formula cell inside the ranges) +/// 2 2 TRUE #DIV/0! (E2 only when `error` is set) +/// 3 text 3 +/// 4 -0.25 +/// ``` +fn aggregate_of(error: bool, formula: &str) -> Value { + let mut t = T::new(); + for (at, v) in [("A1", "1.5"), ("A2", "2"), ("A3", "skipped"), ("A4", "-0.25"), ("B1", "10"), ("B2", "TRUE"), ("B3", "3"), ("C1", "=A1*4")] { + t.set(at, v); + } + if error { + t.set("E2", "#DIV/0!"); + } + + // AB1 is outside every range used, so the formula can't read itself. + t.set("AB1", formula); + t.get("AB1") +} + +#[test] +fn sum_count_average_over_ranges() { + // Text, booleans, blanks and a formula cell inside ranges; direct arguments; unions; whole + // columns; blocks holding an error or nothing numeric. The expected values are what the + // array path (the ranges copied into an array) gave before the ranges were folded in place, + // so any difference is a change of behavior. + let n = Value::Number; + let e = Value::Error; + + // (formula written with SUM, error in E2?, [SUM, COUNT, AVERAGE] of that formula) + #[rustfmt::skip] + let cases: &[(&str, bool, [Value; 3])] = &[ + ("=SUM(A1:A1,B3)", false, [n(4.5), n(2.0), n(2.25)]), + ("=SUM(A1:A1,B3)", true, [n(4.5), n(2.0), n(2.25)]), + ("=SUM((A1:A2,B1:B3))", false, [n(16.5), n(4.0), n(4.125)]), + ("=SUM((A1:A2,B1:B3))", true, [n(16.5), n(4.0), n(4.125)]), + ("=SUM(A1:A4,B1:B3)", false, [n(16.25), n(5.0), n(3.25)]), + ("=SUM(A1:A4,B1:B3)", true, [n(16.25), n(5.0), n(3.25)]), + ("=SUM(A1:A4,\"x\")", false, [e(CellError::Value), n(3.0), e(CellError::Value)]), + ("=SUM(A1:A4,\"x\")", true, [e(CellError::Value), n(3.0), e(CellError::Value)]), + ("=SUM(A1:B4,5,\"7\",TRUE)", false, [n(29.25), n(8.0), n(3.65625)]), + ("=SUM(A1:B4,5,\"7\",TRUE)", true, [n(29.25), n(8.0), n(3.65625)]), + ("=SUM(A1:C4)+SUM(B1:B3)", false, [n(35.25), n(8.0), n(10.208333333333334)]), + ("=SUM(A1:C4)+SUM(B1:B3)", true, [n(35.25), n(8.0), n(10.208333333333334)]), + ("=SUM(A1:C4)", false, [n(22.25), n(6.0), n(3.7083333333333335)]), + ("=SUM(A1:C4)", true, [n(22.25), n(6.0), n(3.7083333333333335)]), + ("=SUM(A1:E4)", false, [n(22.25), n(6.0), n(3.7083333333333335)]), + ("=SUM(A1:E4)", true, [e(CellError::Div0), n(6.0), e(CellError::Div0)]), + ("=SUM(A1)", false, [n(1.5), n(1.0), n(1.5)]), + ("=SUM(A1)", true, [n(1.5), n(1.0), n(1.5)]), + ("=SUM(A3,B2)", false, [n(0.0), n(0.0), e(CellError::Div0)]), + ("=SUM(A3,B2)", true, [n(0.0), n(0.0), e(CellError::Div0)]), + ("=SUM(A:C)", false, [n(22.25), n(6.0), n(3.7083333333333335)]), + ("=SUM(A:C)", true, [n(22.25), n(6.0), n(3.7083333333333335)]), + ("=SUM(E1:E3)", false, [n(0.0), n(0.0), e(CellError::Div0)]), + ("=SUM(E1:E3)", true, [e(CellError::Div0), n(0.0), e(CellError::Div0)]), + ("=SUM(Z1:Z9)", false, [n(0.0), n(0.0), e(CellError::Div0)]), + ("=SUM(Z1:Z9)", true, [n(0.0), n(0.0), e(CellError::Div0)]), + ]; + + for (formula, error, want) in cases { + for (func, want) in ["SUM", "COUNT", "AVERAGE"].into_iter().zip(want) { + // The same formula for each function: `SUM(` becomes `COUNT(` or `AVERAGE(`. + let f = formula.replace("SUM", func); + + assert_eq!(&aggregate_of(*error, &f), want, "{f} (error in E2: {error})"); + } + } +} + +#[test] +fn sum_count_average_have_no_size_limit() { + // One stored cell far down stretches the used range to 27 million cells (27 columns by a + // million rows), past the 16M-cell limit of ranges copied into an array. Only two cells are + // stored, so the sheet itself is tiny. + for (func, want) in [("SUM", Value::Number(2.5)), ("COUNT", Value::Number(2.0)), ("AVERAGE", Value::Number(1.25))] { + for range in ["A1:AA1000000", "A:AA"] { + let mut t = T::new(); + t.set("A1", "1.5"); + t.set("Z1000000", "1"); + + t.set("AB1", &format!("={func}({range})")); + + assert_eq!(t.get("AB1"), want, "{func}({range})"); + } + } +} + +#[test] +fn sum_count_average_over_spilled_values() { + // A1 spills over A1:A3, so the block holds values that exist only as a spill (they have no + // stored cell of their own). The host must not fold such a block in place: it falls back + // to the array path, which sees the spill. Expected: 1 + 2 + 3 + 10. + let mut t = T::new(); + t.set("A1", "=SEQUENCE(3)"); + t.set("A4", "10"); + + for (f, want) in [("SUM", 16.0), ("COUNT", 4.0), ("AVERAGE", 4.0)] { + t.set("C1", &format!("={f}(A1:A5)")); + + assert_eq!(t.num("C1"), want, "{f}"); + } +} + +#[test] +fn aggregate_arguments_are_read_in_order() { + // Volatile cells draw a random number each time they are calculated, so the order in which + // the arguments are read decides which draw each one gets. `B1:B2` comes first, so B2 must + // draw before the `RAND()` argument does. (Reading the plain `RAND()` first once gave + // 7.9043 here.) The expected values are what the array path gave; they depend on the random + // generator and its seed, and must be updated if either changes. + let mut t = T::new(); + t.set("B1", "1"); + t.set("B2", "=RAND()*10"); + + t.set("A1", "=SUM(B1:B2,RAND())"); + + assert_eq!((t.num("A1"), t.num("B2")), (4.968520165759784, 3.3112028100185356)); +} + +#[test] +fn sum_count_average_follow_edits_inside_the_range() { + // A range read in place must still count as a precedent of the formula: if it didn't, an + // edit inside it would leave the formula showing a stale value. Each step below changes one + // cell inside the range and checks all three functions; the last one changes a cell outside. + for range in ["A1:A5", "A:A"] { + let mut t = T::new(); + t.set("A1", "1"); + t.set("A3", "3"); + t.set("C1", &format!("=SUM({range})")); + t.set("C2", &format!("=COUNT({range})")); + t.set("C3", &format!("=AVERAGE({range})")); + let all = |t: &T| (t.num("C1"), t.num("C2"), t.num("C3")); + assert_eq!(all(&t), (4.0, 2.0, 2.0), "{range}"); + + t.set("A1", "5"); + assert_eq!(all(&t), (8.0, 2.0, 4.0), "{range}: edit"); + + t.set("A2", "10"); + assert_eq!(all(&t), (18.0, 3.0, 6.0), "{range}: fill a blank"); + + t.set("A3", "text"); + assert_eq!(all(&t), (15.0, 2.0, 7.5), "{range}: number to text"); + + t.set("A2", ""); + assert_eq!(all(&t), (5.0, 1.0, 5.0), "{range}: clear"); + + t.set("A5", "=A1*2"); + assert_eq!(all(&t), (15.0, 2.0, 7.5), "{range}: formula in the range"); + + t.set("A1", "1"); + assert_eq!(all(&t), (3.0, 2.0, 1.5), "{range}: its precedent changes"); + + // A7 is inside the whole column A:A but outside A1:A5. + t.set("A7", "100"); + let outside = if range == "A:A" { (103.0, 3.0, 103.0 / 3.0) } else { (3.0, 2.0, 1.5) }; + assert_eq!(all(&t), outside, "{range}: a cell outside the range"); + } +} + +#[test] +fn visit_range_declines_when_a_formula_in_the_block_spills_into_it() { + // The sheet: D1 = SEQUENCE(3) and D4 = 10, and D1 has not been calculated yet when the + // block D1:D5 is read. + let mut wb = Workbook::new(); + let sh = wb.sheet_mut(0).unwrap(); + sh.cells.set(c("D1"), Cell::formula(Formula::new("=SEQUENCE(3)"))); + sh.cells.set(c("D4"), Cell::value(Value::Number(10.0))); + let block = gridcraft_core::RangeRef::parse("D1:D5").unwrap(); + + // Reading the block calculates D1, which spills over D2:D3. A walk over the stored cells + // would never see 2 and 3, so the host must decline and leave the block to `range_values`. + // (`Calc` recalculates a pass that produced new spills, so final results are right either + // way; only this direct call shows the host's contract.) + let walked = crate::recalc::tests::probe_visit_range(&wb, &[("D1", "=SEQUENCE(3)")], block); + + assert_eq!(walked, None, "walked a block holding a spill it can't see"); +}