refactor(core): evaluate_aggregated takes Aggregation, not a list it cannot read
The parameter was none_cats: &[String] and the body consulted it exactly once,
for emptiness -- a boolean wearing a list's type, since the aggregation runs
over key.0 through matching_values. git log -S back to the commit that
introduced Axis::None shows it never examined an element, so the name promised a
filter the function has never performed.
Aggregation::{Exact, OverHidden} admits only the two states the body
distinguishes, and Aggregation::of reduces the list once at the boundary that
holds it. The regression guard is the signature: two lists with the same
emptiness and different contents is no longer a constructible call.
recompute_formulas and aggregate_raw keep &[String] -- they walk the elements.
improvise-5jm5
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -39,6 +39,29 @@ differences are the whole content of the design (improvise-kdp):
|
||||
for `SUM`/`COUNT`, which have an identity, and an error for
|
||||
`AVG`/`MIN`/`MAX`, which do not.
|
||||
|
||||
### `evaluate_aggregated` takes `Aggregation`, not a list it cannot read
|
||||
|
||||
The parameter was `none_cats: &[String]`, and the body consulted it exactly
|
||||
once, for emptiness — a boolean wearing a list's type, since the aggregation
|
||||
itself runs over `key.0` through `matching_values`. It never filtered by that
|
||||
list, in any revision: `git log -S` back to the commit that introduced
|
||||
`Axis::None` shows the same `is_empty()` and nothing else. So the name promised
|
||||
a filter the body has never performed, and the next caller to build a careful
|
||||
list would have threaded it into a parameter that cannot use it.
|
||||
|
||||
`Aggregation::{Exact, OverHidden}` admits only the two states the body
|
||||
distinguishes, and `Aggregation::of(none_cats)` performs the reduction once, at
|
||||
the boundary that holds the list (improvise-5jm5,
|
||||
`doctrine(an_invariant_is_made_unrepresentable_not_validated)` — the same answer
|
||||
`prune_axis_items` and `gp-tree`'s `Row<T>` got). It is an enum rather than a
|
||||
`bool` because the two states have names worth reading at the call site, and the
|
||||
regression guard is the **signature**: the thing the ticket describes — two
|
||||
different lists, same emptiness, same answer — is no longer constructible.
|
||||
|
||||
Note what does *not* change: `recompute_formulas` and `aggregate_raw` keep
|
||||
`&[String]`, because they walk the elements (stripping each hidden coordinate
|
||||
off a stem). Two parameters that looked alike were doing two different jobs.
|
||||
|
||||
### Non-numeric values reduce by unanimity, and the branch order is the rule
|
||||
|
||||
`aggregate_cell_values` checks the **all-numeric branch first**. If every value
|
||||
|
||||
@@ -12,6 +12,20 @@ none.
|
||||
|
||||
## `model/types.rs`
|
||||
|
||||
### a_hidden_dimension_list_reduces_to_one_of_two_states
|
||||
|
||||
improvise-5jm5. `evaluate_aggregated` took `none_cats: &[String]` and consulted
|
||||
it once, for emptiness — so two lists with different contents and the same
|
||||
emptiness gave the same answer, while the name promised a filter. Nothing
|
||||
misbehaved; the signature described something the body never did.
|
||||
|
||||
This test is the *smaller* half of the guard and says so. The real guard is the
|
||||
signature: the defect the ticket describes is no longer constructible, exactly
|
||||
as `Checked`'s constructor is the guard for the basis check rather than a test
|
||||
that can fail. What this test does add is that `Aggregation::of` is the only
|
||||
place the list is read, and that a list of two hidden dimensions is not somehow
|
||||
a third state — the reduction is emptiness, and the enum is not a count.
|
||||
|
||||
### apply_agg_over_nothing_is_absent_for_every_function
|
||||
|
||||
An empty cell must read as *absent*, not as zero — otherwise every blank
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use crate::model::Aggregation;
|
||||
use crate::ui::effect::{self, Effect};
|
||||
|
||||
use super::core::{Cmd, CmdContext};
|
||||
@@ -140,7 +141,9 @@ impl Cmd for YankCell {
|
||||
"yank"
|
||||
}
|
||||
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
|
||||
let value = ctx.model.evaluate_aggregated(&self.key, ctx.none_cats());
|
||||
let value = ctx
|
||||
.model
|
||||
.evaluate_aggregated(&self.key, Aggregation::of(ctx.none_cats()));
|
||||
vec![
|
||||
Box::new(effect::SetYanked(value)),
|
||||
effect::set_status("Yanked"),
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use crate::model::Aggregation;
|
||||
use crate::model::cell::CellValue;
|
||||
use crate::ui::app::AppMode;
|
||||
use crate::ui::effect::{self, Effect, Panel};
|
||||
@@ -112,7 +113,10 @@ impl Cmd for SearchNavigate {
|
||||
Some(k) => k,
|
||||
None => return false,
|
||||
};
|
||||
let s = match ctx.model.evaluate_aggregated(&key, ctx.none_cats()) {
|
||||
let s = match ctx
|
||||
.model
|
||||
.evaluate_aggregated(&key, Aggregation::of(ctx.none_cats()))
|
||||
{
|
||||
Some(CellValue::Number(n)) => format!("{n}"),
|
||||
Some(CellValue::Text(t)) => t,
|
||||
Some(CellValue::Error(e)) => format!("ERR:{e}"),
|
||||
|
||||
@@ -4,10 +4,10 @@
|
||||
use std::collections::BTreeMap;
|
||||
|
||||
use improvise_core::format::{format_f64, format_value, parse_number_format};
|
||||
use improvise_core::model::Model;
|
||||
use improvise_core::model::category::MEASURE_CATEGORY;
|
||||
use improvise_core::model::cell::CellValue;
|
||||
use improvise_core::model::record::{AggregateHash, RecordHash};
|
||||
use improvise_core::model::{Aggregation, Model};
|
||||
use improvise_core::view::layout::{AxisEntry, GridLayout, RecordCol};
|
||||
use improvise_core::view::{Axis, View};
|
||||
use improvise_core::workbook::Workbook;
|
||||
@@ -233,7 +233,9 @@ fn column_totals(
|
||||
.map(|c| {
|
||||
let total: f64 = (0..layout.row_count())
|
||||
.filter_map(|r| layout.cell_key(r, c))
|
||||
.map(|key| model.evaluate_aggregated_f64(&key, &layout.none_cats))
|
||||
.map(|key| {
|
||||
model.evaluate_aggregated_f64(&key, Aggregation::of(&layout.none_cats))
|
||||
})
|
||||
.sum();
|
||||
CellDisplay::number(format_f64(total, comma, decimals))
|
||||
})
|
||||
@@ -268,7 +270,7 @@ fn cell_display(
|
||||
app.model_state
|
||||
.workbook
|
||||
.model
|
||||
.evaluate_aggregated(&key, &layout.none_cats)
|
||||
.evaluate_aggregated(&key, Aggregation::of(&layout.none_cats))
|
||||
});
|
||||
CellDisplay {
|
||||
text: format_value(value.as_ref(), comma, decimals),
|
||||
|
||||
@@ -6,7 +6,7 @@ use unicode_width::UnicodeWidthStr;
|
||||
use improvise_protocol::header_level_widths;
|
||||
|
||||
use crate::format::format_f64;
|
||||
use crate::model::Model;
|
||||
use crate::model::{Aggregation, Model};
|
||||
use crate::view::{AxisEntry, GridLayout};
|
||||
|
||||
/// Minimum column width — enough for short numbers/labels + 1 char gap.
|
||||
@@ -62,7 +62,7 @@ pub fn compute_col_widths(
|
||||
for (ci, wref) in widths.iter_mut().enumerate().take(n) {
|
||||
let total: f64 = (0..layout.row_count())
|
||||
.filter_map(|ri| layout.cell_key(ri, ci))
|
||||
.map(|key| model.evaluate_aggregated_f64(&key, &layout.none_cats))
|
||||
.map(|key| model.evaluate_aggregated_f64(&key, Aggregation::of(&layout.none_cats)))
|
||||
.sum();
|
||||
let s = format_f64(total, fmt_comma, fmt_decimals);
|
||||
let w = s.width() as u16;
|
||||
|
||||
@@ -4,4 +4,4 @@ pub mod record;
|
||||
pub mod symbol;
|
||||
pub mod types;
|
||||
|
||||
pub use types::Model;
|
||||
pub use types::{Aggregation, Model};
|
||||
|
||||
@@ -202,6 +202,25 @@ fn eval_is(context: &CellKey, filter: &Filter) -> f64 {
|
||||
|
||||
/// Pure-data document model: categories, cells, formulas. It knows nothing
|
||||
/// about views — see [`crate::workbook::Workbook`] and core.md §10.
|
||||
/// Whether a grid hides dimensions a cell key does not name. The emptiness of
|
||||
/// the view's `Axis::None` list is the whole question — `context/code/core.md`.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum Aggregation {
|
||||
/// The key names every dimension, so the cell is read as stored.
|
||||
Exact,
|
||||
/// Fold every cell the key does not name.
|
||||
OverHidden,
|
||||
}
|
||||
|
||||
impl Aggregation {
|
||||
pub fn of(none_cats: &[String]) -> Self {
|
||||
match none_cats.is_empty() {
|
||||
true => Aggregation::Exact,
|
||||
false => Aggregation::OverHidden,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
pub struct Model {
|
||||
pub name: String,
|
||||
@@ -476,8 +495,8 @@ impl Model {
|
||||
})
|
||||
}
|
||||
|
||||
pub fn evaluate_aggregated(&self, key: &CellKey, none_cats: &[String]) -> Option<CellValue> {
|
||||
if none_cats.is_empty() {
|
||||
pub fn evaluate_aggregated(&self, key: &CellKey, agg: Aggregation) -> Option<CellValue> {
|
||||
if agg == Aggregation::Exact {
|
||||
return self.evaluate(key);
|
||||
}
|
||||
|
||||
@@ -491,8 +510,8 @@ impl Model {
|
||||
}
|
||||
|
||||
/// [`Self::evaluate_aggregated`] as `f64`, with `0.0` for an empty cell.
|
||||
pub fn evaluate_aggregated_f64(&self, key: &CellKey, none_cats: &[String]) -> f64 {
|
||||
self.evaluate_aggregated(key, none_cats)
|
||||
pub fn evaluate_aggregated_f64(&self, key: &CellKey, agg: Aggregation) -> f64 {
|
||||
self.evaluate_aggregated(key, agg)
|
||||
.and_then(|v| v.as_f64())
|
||||
.unwrap_or(0.0)
|
||||
}
|
||||
@@ -815,7 +834,7 @@ mod agg_helper_tests {
|
||||
|
||||
#[cfg(test)]
|
||||
mod model_tests {
|
||||
use super::Model;
|
||||
use super::{Aggregation, Model};
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
|
||||
fn coord(pairs: &[(&str, &str)]) -> CellKey {
|
||||
@@ -994,7 +1013,7 @@ mod model_tests {
|
||||
|
||||
// With Date hidden, it aggregates to SUM.
|
||||
let none_cats = vec!["Date".to_string()];
|
||||
let result = m.evaluate_aggregated(&partial_key, &none_cats);
|
||||
let result = m.evaluate_aggregated(&partial_key, Aggregation::of(&none_cats));
|
||||
assert_eq!(result, Some(CellValue::Number(150.0)));
|
||||
}
|
||||
|
||||
@@ -1031,6 +1050,19 @@ mod model_tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_hidden_dimension_list_reduces_to_one_of_two_states() {
|
||||
assert_eq!(Aggregation::of(&[]), Aggregation::Exact);
|
||||
assert_eq!(
|
||||
Aggregation::of(&["Date".to_string()]),
|
||||
Aggregation::OverHidden
|
||||
);
|
||||
assert_eq!(
|
||||
Aggregation::of(&["Date".to_string(), "Payee".to_string()]),
|
||||
Aggregation::OverHidden
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn evaluate_aggregated_no_hidden_delegates_to_evaluate() {
|
||||
let mut m = Model::new("Test");
|
||||
@@ -1039,7 +1071,7 @@ mod model_tests {
|
||||
m.set_cell(coord(&[("Region", "East")]), CellValue::Number(42.0));
|
||||
let key = coord(&[("Region", "East")]);
|
||||
assert_eq!(
|
||||
m.evaluate_aggregated(&key, &[]),
|
||||
m.evaluate_aggregated(&key, Aggregation::Exact),
|
||||
Some(CellValue::Number(42.0))
|
||||
);
|
||||
}
|
||||
@@ -1065,7 +1097,7 @@ mod model_tests {
|
||||
fn status_of(m: &Model) -> Option<CellValue> {
|
||||
m.evaluate_aggregated(
|
||||
&coord(&[("Payee", "Acme"), ("_Measure", "Status")]),
|
||||
&["Date".to_string()],
|
||||
Aggregation::of(&["Date".to_string()]),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -1156,14 +1188,14 @@ mod model_tests {
|
||||
|
||||
let key = coord(&[("Payee", "Acme"), ("_Measure", "Price")]);
|
||||
let none_cats = vec!["Date".to_string()];
|
||||
let result = m.evaluate_aggregated(&key, &none_cats);
|
||||
let result = m.evaluate_aggregated(&key, Aggregation::of(&none_cats));
|
||||
assert_eq!(result, Some(CellValue::Number(20.0))); // avg(10, 30) = 20
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod formula_tests {
|
||||
use super::Model;
|
||||
use super::{Aggregation, Model};
|
||||
use crate::formula::parse_formula;
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
|
||||
@@ -1715,12 +1747,18 @@ mod formula_tests {
|
||||
m.recompute_formulas(&none_cats);
|
||||
// Equality says X is zero…
|
||||
assert_eq!(
|
||||
m.evaluate_aggregated(&coord(&[("_Measure", "IsZero")]), &none_cats),
|
||||
m.evaluate_aggregated(
|
||||
&coord(&[("_Measure", "IsZero")]),
|
||||
Aggregation::of(&none_cats)
|
||||
),
|
||||
Some(CellValue::Number(1.0))
|
||||
);
|
||||
// …so dividing by X must be div/0 in the cached path too.
|
||||
assert_eq!(
|
||||
m.evaluate_aggregated(&coord(&[("_Measure", "Ratio")]), &none_cats),
|
||||
m.evaluate_aggregated(
|
||||
&coord(&[("_Measure", "Ratio")]),
|
||||
Aggregation::of(&none_cats)
|
||||
),
|
||||
Some(CellValue::Error("div/0".into()))
|
||||
);
|
||||
}
|
||||
@@ -2382,7 +2420,7 @@ mod route_then_aggregate_tests {
|
||||
|
||||
let uncached = m.evaluate(&north);
|
||||
m.recompute_formulas(&none_cats);
|
||||
let cached = m.evaluate_aggregated(&north, &none_cats);
|
||||
let cached = m.evaluate_aggregated(&north, Aggregation::of(&none_cats));
|
||||
|
||||
assert_eq!(uncached, Some(CellValue::Number(11.0)));
|
||||
assert_eq!(cached, uncached, "the two evaluators must not diverge");
|
||||
@@ -2397,7 +2435,10 @@ mod route_then_aggregate_tests {
|
||||
m.recompute_formulas(&none_cats);
|
||||
for region in ["North", "South"] {
|
||||
assert_eq!(
|
||||
m.evaluate_aggregated(&key(&[("Region", region), (MEASURE, "Pred")]), &none_cats),
|
||||
m.evaluate_aggregated(
|
||||
&key(&[("Region", region), (MEASURE, "Pred")]),
|
||||
Aggregation::of(&none_cats)
|
||||
),
|
||||
Some(CellValue::Number(11.0)),
|
||||
"{region}"
|
||||
);
|
||||
@@ -2428,7 +2469,10 @@ mod route_then_aggregate_tests {
|
||||
m.recompute_formulas(&none_cats);
|
||||
for (region, expected) in [("North", 1.0), ("South", 0.0)] {
|
||||
assert_eq!(
|
||||
m.evaluate_aggregated(&key(&[("Region", region), (MEASURE, "Pred")]), &none_cats),
|
||||
m.evaluate_aggregated(
|
||||
&key(&[("Region", region), (MEASURE, "Pred")]),
|
||||
Aggregation::of(&none_cats)
|
||||
),
|
||||
Some(CellValue::Number(expected)),
|
||||
"{region}"
|
||||
);
|
||||
@@ -2444,7 +2488,10 @@ mod route_then_aggregate_tests {
|
||||
m.recompute_formulas(&none_cats);
|
||||
for (region, expected) in [("North", 14.0), ("South", 200.0)] {
|
||||
assert_eq!(
|
||||
m.evaluate_aggregated(&key(&[("Region", region), (MEASURE, "Pred")]), &none_cats),
|
||||
m.evaluate_aggregated(
|
||||
&key(&[("Region", region), (MEASURE, "Pred")]),
|
||||
Aggregation::of(&none_cats)
|
||||
),
|
||||
Some(CellValue::Number(expected)),
|
||||
"{region}"
|
||||
);
|
||||
|
||||
@@ -1,8 +1,8 @@
|
||||
use std::rc::Rc;
|
||||
|
||||
use crate::model::Model;
|
||||
use crate::model::category::{DIM_CATEGORY, INDEX_CATEGORY, MEASURE_CATEGORY};
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
use crate::model::{Aggregation, Model};
|
||||
use crate::view::{Axis, View};
|
||||
|
||||
/// The trailing column that creates a new measure: typing a name into it
|
||||
@@ -427,7 +427,7 @@ impl GridLayout {
|
||||
for (ci, cell) in row.iter_mut().enumerate() {
|
||||
*cell = self
|
||||
.cell_key(ri, ci)
|
||||
.and_then(|k| model.evaluate_aggregated(&k, &self.none_cats))
|
||||
.and_then(|k| model.evaluate_aggregated(&k, Aggregation::of(&self.none_cats)))
|
||||
.is_some();
|
||||
}
|
||||
}
|
||||
@@ -485,7 +485,7 @@ impl GridLayout {
|
||||
self.records_display(row, col).unwrap_or_default()
|
||||
} else {
|
||||
self.cell_key(row, col)
|
||||
.and_then(|key| model.evaluate_aggregated(&key, &self.none_cats))
|
||||
.and_then(|key| model.evaluate_aggregated(&key, Aggregation::of(&self.none_cats)))
|
||||
.map(|v| crate::format::format_value(Some(&v), fmt_comma, fmt_decimals))
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
@@ -470,7 +470,11 @@ mod tests {
|
||||
assert!(parse_formula("X = Revenue WHEREAS=y", "Cat").is_err());
|
||||
assert!(parse_formula("X = SUM(Rev WHEREAS=y)", "Cat").is_err());
|
||||
let inline = parse_formula("X = SUM(Rev WHERE Region = East)", "Cat").unwrap();
|
||||
assert!(matches!(&inline.expr, Expr::Agg(_, _, Some(_))), "{:?}", inline.expr);
|
||||
assert!(
|
||||
matches!(&inline.expr, Expr::Agg(_, _, Some(_))),
|
||||
"{:?}",
|
||||
inline.expr
|
||||
);
|
||||
let trailing = parse_formula("X = Revenue WHERE Region = East", "Cat").unwrap();
|
||||
assert_eq!(trailing.filter.as_ref().unwrap().category, "Region");
|
||||
}
|
||||
|
||||
@@ -297,7 +297,10 @@ mod tests {
|
||||
fn value_at(wb: &Workbook, row: usize, col: usize) -> Option<CellValue> {
|
||||
let layout = improvise_core::view::layout::GridLayout::new(&wb.model, wb.active_view());
|
||||
let key = layout.cell_key(row, col)?;
|
||||
wb.model.evaluate_aggregated(&key, &layout.none_cats)
|
||||
wb.model.evaluate_aggregated(
|
||||
&key,
|
||||
improvise_core::model::Aggregation::of(&layout.none_cats),
|
||||
)
|
||||
}
|
||||
|
||||
/// `Storage::list` is how a browser offers a document picker, and it must
|
||||
|
||||
@@ -5,6 +5,7 @@ use std::path::{Path, PathBuf};
|
||||
|
||||
use improvise_command::ui::app::App;
|
||||
use improvise_command::ui::reduce;
|
||||
use improvise_core::model::Aggregation;
|
||||
use improvise_core::model::cell::{CellKey, CellValue};
|
||||
use improvise_core::view::Axis;
|
||||
use improvise_core::view::layout::GridLayout;
|
||||
@@ -190,7 +191,8 @@ fn the_worker_loads_projects_edits_and_persists_over_post_message() {
|
||||
fn value_at(wb: &Workbook, row: usize, col: usize) -> Option<CellValue> {
|
||||
let layout = GridLayout::new(&wb.model, wb.active_view());
|
||||
let key = layout.cell_key(row, col)?;
|
||||
wb.model.evaluate_aggregated(&key, &layout.none_cats)
|
||||
wb.model
|
||||
.evaluate_aggregated(&key, Aggregation::of(&layout.none_cats))
|
||||
}
|
||||
|
||||
/// A second worker over the same backend sees what the first one saved: what a
|
||||
|
||||
Reference in New Issue
Block a user