From 34dd0247bf6c4edc89e6acfcb6f54d913203865a Mon Sep 17 00:00:00 2001 From: alehel Date: Wed, 11 Nov 2026 09:17:28 +0000 Subject: [PATCH 1/4] pivot: one shared module for a PivotTable's source, headers and item labels model::pivot_source resolves a PivotTable's source (a table without its totals row, a range, or a defined name, sheet-scoped or chained), names its columns (trimmed, repeats numbered) and labels its items (column format, date groups, (blank)). The engine's reports call it instead of their own copies, so file writers can state the same columns and items. PivotTable::axis_field replaces the engine's page-field grouping lookup, and numfmt's Formatted::plain replaces the display code's inline fill handling. Column headers are now read as values even while Show Formulas is on, and a repeat-fill in a number format shows as one space in item labels. --- Cargo.lock | 1 + crates/engine/src/cmd/pivot/tests.rs | 71 +++++++++ crates/engine/src/display.rs | 12 +- crates/engine/src/pivot.rs | 79 +--------- crates/engine/src/pivot/compute.rs | 4 +- crates/engine/src/pivot/items.rs | 42 ++--- crates/model/Cargo.toml | 1 + crates/model/src/features/pivot.rs | 8 + crates/model/src/lib.rs | 1 + crates/model/src/pivot_source.rs | 224 +++++++++++++++++++++++++++ crates/numfmt/src/lib.rs | 15 ++ crates/numfmt/src/tests.rs | 7 + docs/architecture.md | 5 +- 13 files changed, 351 insertions(+), 119 deletions(-) create mode 100644 crates/model/src/pivot_source.rs diff --git a/Cargo.lock b/Cargo.lock index 20dcf28abc..e53c3c406a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2106,6 +2106,7 @@ dependencies = [ "gridcraft-core", "gridcraft-formula", "gridcraft-math", + "gridcraft-numfmt", "serde", "serde_json", "sha2 0.11.1", diff --git a/crates/engine/src/cmd/pivot/tests.rs b/crates/engine/src/cmd/pivot/tests.rs index 3748d07054..88e5d2ed10 100644 --- a/crates/engine/src/cmd/pivot/tests.rs +++ b/crates/engine/src/cmd/pivot/tests.rs @@ -1059,3 +1059,74 @@ fn legacy_count_numbers_caption_follows_the_function() { assert_eq!(text(&s, "E3"), after, "{legacy}"); } } + +/// The grand total of the report on the active sheet (column `col` of the Grand Total row). +fn grand_total(s: &Session, col: u32) -> Value { + let gt = find_row(s, "Grand Total").unwrap(); + val(s, &CellRef::new(gt, col).a1()) +} + +#[test] +fn table_source_leaves_out_its_totals_row() { + let mut s = sales(); + s.execute("insert.table", json!({"range": "A1:E13", "name": "SalesTable"})).unwrap(); + s.execute("table.totalRow", json!({"table": "SalesTable", "on": true})).unwrap(); + s.execute("insert.pivotTable", json!({"source": "SalesTable", "rows": ["Region"], "values": ["Sales"]})).unwrap(); + // Three regions, no "Total" item, and the totals row's sum is not counted again. + assert_eq!(find_row(&s, "Total"), None); + assert_eq!(grand_total(&s, 1), Value::Number(TOTAL)); + assert_eq!(pt(&s).last_range.unwrap().height(), 5); +} + +#[test] +fn sheet_scoped_and_chained_names_resolve() { + let mut s = sales(); + s.execute("formulas.defineName", json!({"name": "Data", "refersTo": "=Sheet1!$A$1:$E$13", "scope": "Sheet1"})).unwrap(); + s.execute("formulas.defineName", json!({"name": "Alias", "refersTo": "=Data"})).unwrap(); + for (source, at) in [("Data", "H1"), ("Alias", "H20")] { + s.execute("insert.pivotTable", json!({"source": source, "destination": format!("Sheet1!{at}"), "rows": ["Region"], "values": ["Sales"]})) + .unwrap(); + assert_eq!(pt(&s).source, source); + let r = pt(&s).last_range.unwrap(); + assert_eq!(val(&s, &CellRef::new(r.end.row, r.start.col + 1).a1()), Value::Number(TOTAL), "{source}"); + } +} + +#[test] +fn repeated_and_padded_headers_keep_their_fields() { + let mut s = Session::new(); + s.new_workbook(); + let rows = json!([[" Region ", "Sales", "sales"], ["East", 1, 10], ["West", 2, 20], ["East", 3, 30]]); + s.execute("range.setValues", json!({"range": "A1", "values": rows})).unwrap(); + let r = + s.execute("insert.pivotTable", json!({"source": "Sheet1!A1:C4", "rows": ["Region"], "values": ["sales2"], "filters": ["Sales"]})).unwrap(); + assert_eq!(r["fields"], json!(["Region", "Sales", "sales2"])); + // The page field on the first `Sales` stays above the report and filters by its own column. + assert_eq!(text(&s, "A1"), "Sales"); + s.execute("pivot.filter", json!({"field": "Sales", "selected": ["1", "3"]})).unwrap(); + assert_eq!(text(&s, "B1"), "(Multiple Items)"); + assert_eq!(grand_total(&s, 1), Value::Number(40.0)); +} + +#[test] +fn formatted_page_items_are_chosen_by_their_labels() { + let mut s = sales(); + s.execute("home.numberFormat", json!({"range": "E2:E13", "code": "$#,##0.00"})).unwrap(); + s.execute("insert.pivotTable", json!({"rows": ["Region"], "values": ["Units"], "filters": ["Sales"]})).unwrap(); + s.execute("pivot.filter", json!({"field": "Sales", "selected": ["$120.00"]})).unwrap(); + assert_eq!(text(&s, "B1"), "$120.00"); + // East Pears (8) and West Apples (12). + assert_eq!(grand_total(&s, 1), Value::Number(20.0)); + s.execute("pivot.filter", json!({"field": "Sales", "selected": ["$120.00", "$60.00"]})).unwrap(); + assert_eq!(text(&s, "B1"), "(Multiple Items)"); + assert_eq!(grand_total(&s, 1), Value::Number(30.0)); +} + +#[test] +fn headers_read_values_while_formulas_show() { + let mut s = sales(); + s.execute("cell.set", json!({"cell": "A1", "input": "=\"Reg\"&\"ion\""})).unwrap(); + s.execute("formulas.showFormulas", json!({"on": true})).unwrap(); + let r = s.execute("insert.pivotTable", json!({"rows": ["Region"], "values": ["Sales"]})).unwrap(); + assert_eq!(r["fields"][0], "Region"); +} diff --git a/crates/engine/src/display.rs b/crates/engine/src/display.rs index 06a0fa5875..cb903d40a0 100644 --- a/crates/engine/src/display.rs +++ b/crates/engine/src/display.rs @@ -127,17 +127,7 @@ pub fn value_text(wb: &Workbook, sheet: &Sheet, c: CellRef) -> String { /// [`value_text`] for a value and a format code given apart from a cell (print takes the code /// from the cell's effective look, which a conditional format can override). pub fn value_text_as(wb: &Workbook, v: &Value, code: &str) -> String { - let f = format(v, code, wb); - match f.fill { - Some((_, pos)) => { - let mut t = f.text; - if pos <= t.len() && t.is_char_boundary(pos) { - t.insert(pos, ' '); - } - t - } - None => f.text, - } + format(v, code, wb).plain() } /// Tab-separated text of a range, as the system clipboard gets it. diff --git a/crates/engine/src/pivot.rs b/crates/engine/src/pivot.rs index bfaccace47..aacd639b2c 100644 --- a/crates/engine/src/pivot.rs +++ b/crates/engine/src/pivot.rs @@ -8,9 +8,8 @@ use std::cmp::Ordering; use std::collections::{HashMap, HashSet}; -use gridcraft_core::date::datetime_from_serial; pub use gridcraft_core::quote_sheet; -use gridcraft_core::{CellError, CellRef, MAX_COLS, MAX_ROWS, RangeRef, Value, split_sheet}; +use gridcraft_core::{CellError, CellRef, MAX_COLS, MAX_ROWS, RangeRef, Value}; use gridcraft_model::{ BorderLine, BorderStyle, Cell, Chart, Color, Fill, HAlign, NumFmt, PivotDateGroup, PivotField, PivotFilter, PivotFunc, PivotLayout, PivotShowAs, PivotSort, PivotTable, PivotValue, Series, Style, StyleId, Workbook, @@ -42,8 +41,6 @@ pub const MAX_AXIS_FIELDS: usize = 32; /// Most fields in the Values area. pub const MAX_VALUE_FIELDS: usize = 256; -const MONTHS: [&str; 12] = ["Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"]; - // ---------------------------------------------------------------- source /// The source list: header names, per-column number formats and the records. @@ -74,51 +71,9 @@ pub fn range_text(sheet_name: &str, r: RangeRef) -> String { /// Resolves source text (range, table name or defined name) to (sheet, range including header). pub fn resolve_source(wb: &Workbook, text: &str, default_sheet: usize) -> Result<(usize, RangeRef), String> { - resolve_inner(wb, text, default_sheet, 0) -} - -fn resolve_inner(wb: &Workbook, text: &str, default_sheet: usize, depth: u32) -> Result<(usize, RangeRef), String> { - const INVALID: &str = "The reference isn't valid."; - let t = text.trim().trim_start_matches('=').trim(); - if t.is_empty() || depth > 4 { - return Err(INVALID.into()); - } - if let Some((si, ti)) = wb.table(t) - && let Some(tb) = wb.sheet(si).and_then(|s| s.tables.get(ti)) - { - if !tb.header_row { - return Err("The table needs a header row to be used as a PivotTable source.".into()); - } - let end_row = tb.range.end.row.saturating_sub(tb.totals_row as u32).max(tb.range.start.row); - return Ok((si, RangeRef::new(tb.range.start, CellRef::new(end_row, tb.range.end.col)))); - } - let (sheet, body) = split_sheet(t); - if let Some(r) = RangeRef::parse(body) { - let si = match sheet { - Some(n) => wb.sheet_index(&n).ok_or_else(|| INVALID.to_string())?, - None => default_sheet, - }; - let sh = wb.sheet(si).ok_or_else(|| INVALID.to_string())?; - let r = if r.is_full_cols() || r.is_full_rows() { - let used = sh.used_range().ok_or_else(|| INVALID.to_string())?; - r.intersection(&used).ok_or_else(|| INVALID.to_string())? - } else { - r - }; - return Ok((si, r)); - } - if sheet.is_none() - && let Some(n) = wb.name(t, default_sheet) - { - let f = n.formula.clone(); - return resolve_inner(wb, &f, default_sheet, depth + 1); - } - Err(INVALID.into()) + gridcraft_model::pivot_source::resolve(wb, text, default_sheet) } -const FIELD_NAME_ERR: &str = - "The PivotTable field name is not valid. To create a PivotTable, you must use data that is organized as a list with labeled columns."; - /// Reads the source list. pub fn read_source(wb: &Workbook, text: &str, default_sheet: usize) -> Result { let (si, r) = resolve_source(wb, text, default_sheet)?; @@ -126,34 +81,8 @@ pub fn read_source(wb: &Workbook, text: &str, default_sheet: usize) -> Result MAX_SOURCE_CELLS { return Err("The source range is too large for a PivotTable.".into()); } - let mut headers: Vec = Vec::new(); - // Names taken so far (lower-case), and the next suffix to try for each repeated header, so - // that thousands of equal headers don't take cubic time. - let mut taken: HashSet = HashSet::new(); - let mut next_suffix: HashMap = HashMap::new(); - let mut formats = Vec::new(); - for c in r.start.col..=r.end.col { - let h = crate::display::cell_text(wb, sh, CellRef::new(r.start.row, c)).trim().to_string(); - if h.is_empty() { - return Err(FIELD_NAME_ERR.into()); - } - let key = h.to_ascii_lowercase(); - let mut n = h.clone(); - let mut k = next_suffix.get(&key).copied().unwrap_or(2); - while taken.contains(&n.to_ascii_lowercase()) { - n = format!("{h}{k}"); - k += 1; - } - next_suffix.insert(key, k); - taken.insert(n.to_ascii_lowercase()); - headers.push(n); - let fmt = if r.end.row > r.start.row { - wb.styles.get(sh.style_id(CellRef::new(r.start.row + 1, c))).num_fmt.as_str().to_string() - } else { - "General".into() - }; - formats.push(fmt); - } + let headers = gridcraft_model::pivot_source::headers(wb, si, r)?; + let formats: Vec = (r.start.col..=r.end.col).map(|c| gridcraft_model::pivot_source::column_format(wb, si, r, c)).collect(); let w = headers.len(); let mut rows = Vec::new(); let mut numeric = vec![true; w]; diff --git a/crates/engine/src/pivot/compute.rs b/crates/engine/src/pivot/compute.rs index 90e81408d9..a6c43c271e 100644 --- a/crates/engine/src/pivot/compute.rs +++ b/crates/engine/src/pivot/compute.rs @@ -386,14 +386,12 @@ fn show_value(definition: &PivotValue, raw: Value, lookup: &ValueLookup<'_>, con type ReportFilter<'a> = (usize, &'a PivotFilter, PivotDateGroup, bool); fn report_filters<'a>(pt: &'a PivotTable, src: &Source) -> (Vec>, Vec>) { - let axis_group = - |name: &str| pt.rows.iter().chain(pt.columns.iter()).find(|field| field.source_col.eq_ignore_ascii_case(name)).map(|field| field.date_group); let filters: Vec<_> = pt .filters .iter() .filter_map(|filter| { let column = src.col(&filter.source_col)?; - let group = axis_group(&filter.source_col); + let group = pt.axis_field(&filter.source_col).map(|field| field.date_group); Some((column, filter, group.unwrap_or_default(), group.is_none())) }) .collect(); diff --git a/crates/engine/src/pivot/items.rs b/crates/engine/src/pivot/items.rs index 79b74da1ab..a3394e70ec 100644 --- a/crates/engine/src/pivot/items.rs +++ b/crates/engine/src/pivot/items.rs @@ -1,4 +1,4 @@ -use super::{CellError, HashMap, MONTHS, Ordering, PivotDateGroup, PivotField, PivotFunc, PivotSort, Source, Value, Workbook, datetime_from_serial}; +use super::{CellError, HashMap, Ordering, PivotDateGroup, PivotField, PivotFunc, PivotSort, Source, Value, Workbook}; // ---------------------------------------------------------------- items @@ -35,34 +35,20 @@ fn make_item(wb: &Workbook, v: &Value, group: PivotDateGroup, fmt: &str, first: first, formatted, }; + let v = &v.scalar(); + // The label is the model's, which file writers list items by too. + let label = gridcraft_model::pivot_source::item_label(v, group, &crate::display::number_format(fmt), wb.date_system); match v { - Value::Empty => ("b".into(), it(Value::text("(blank)"), "(blank)".into(), 4, 0.0, false)), - Value::Number(n) => { - if group != PivotDateGroup::None - && let Some(dt) = datetime_from_serial(wb.date_system, *n) - { - let mon = MONTHS.get(dt.month.saturating_sub(1) as usize).copied().unwrap_or("Jan"); - let (label, key) = match group { - PivotDateGroup::Years => (dt.year.to_string(), dt.year as f64), - PivotDateGroup::Quarters => { - let q = (dt.month.saturating_sub(1)) / 3 + 1; - (format!("Qtr{q}"), q as f64) - } - PivotDateGroup::Months => (mon.to_string(), dt.month as f64), - _ => (format!("{}-{mon}", dt.day), (dt.month * 100 + dt.day) as f64), - }; - return (format!("g{key}"), it(Value::text(label.as_str()), label, 0, key, false)); - } - let label = crate::display::format(v, fmt, wb).text; - (format!("n{}", n.to_bits()), it(v.clone(), label, 0, *n, true)) - } - Value::Text(t) => (format!("t{}", t.to_lowercase()), it(v.clone(), t.to_string(), 1, 0.0, false)), - Value::Bool(b) => { - let l = if *b { "TRUE" } else { "FALSE" }; - (format!("l{l}"), it(Value::Bool(*b), l.into(), 2, *b as u8 as f64, false)) - } - Value::Error(e) => (format!("e{}", e.as_str()), it(v.clone(), e.as_str().into(), 3, 0.0, false)), - Value::Array(_) => make_item(wb, &v.scalar(), group, fmt, first), + Value::Empty => ("b".into(), it(Value::text("(blank)"), label, 4, 0.0, false)), + Value::Number(n) => match gridcraft_model::pivot_source::date_group_item(wb.date_system, *n, group) { + Some((_, key)) => (format!("g{key}"), it(Value::text(label.as_str()), label, 0, key, false)), + None => (format!("n{}", n.to_bits()), it(v.clone(), label, 0, *n, true)), + }, + Value::Text(t) => (format!("t{}", t.to_lowercase()), it(v.clone(), label, 1, 0.0, false)), + Value::Bool(b) => (format!("l{label}"), it(Value::Bool(*b), label, 2, *b as u8 as f64, false)), + Value::Error(e) => (format!("e{}", e.as_str()), it(v.clone(), label, 3, 0.0, false)), + // An array in an array: no value an item can hold. + Value::Array(_) => ("b".into(), it(Value::text("(blank)"), label, 4, 0.0, false)), } } diff --git a/crates/model/Cargo.toml b/crates/model/Cargo.toml index ff15c67953..67a6261116 100644 --- a/crates/model/Cargo.toml +++ b/crates/model/Cargo.toml @@ -13,6 +13,7 @@ gridcraft-core = { workspace = true } sha2 = { workspace = true } gridcraft-formula = { workspace = true } gridcraft-math = { workspace = true } +gridcraft-numfmt = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } thiserror = { workspace = true } diff --git a/crates/model/src/features/pivot.rs b/crates/model/src/features/pivot.rs index b34015f4cd..1f77a8bf9f 100644 --- a/crates/model/src/features/pivot.rs +++ b/crates/model/src/features/pivot.rs @@ -139,6 +139,14 @@ pub struct PivotTable { pub values_position: Option, } +impl PivotTable { + /// The row or column field on source column `name`: a page field on the same column is + /// shown by it (not above the report) and takes its date grouping. + pub fn axis_field(&self, name: &str) -> Option<&PivotField> { + self.rows.iter().chain(&self.columns).find(|f| f.source_col.eq_ignore_ascii_case(name)) + } +} + /// A calculated field: a formula over other fields (`=Sales*0.1`, `='Unit Price'*Units`). As in /// Excel, it is evaluated on the sums of the fields it names for each report cell, not per record. #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] diff --git a/crates/model/src/lib.rs b/crates/model/src/lib.rs index 35cfa315f8..1beecce0c6 100644 --- a/crates/model/src/lib.rs +++ b/crates/model/src/lib.rs @@ -10,6 +10,7 @@ mod cellmap; pub mod features; pub mod macros; pub mod password; +pub mod pivot_source; pub mod preserved; pub mod python; pub mod sheet; diff --git a/crates/model/src/pivot_source.rs b/crates/model/src/pivot_source.rs new file mode 100644 index 0000000000..eaf53b39a6 --- /dev/null +++ b/crates/model/src/pivot_source.rs @@ -0,0 +1,224 @@ +//! A PivotTable's source as the report reads it: the range a source names, its column names, and +//! the labels its items show. The engine computes reports from these; file writers state them, +//! so both name the same columns and items. + +use std::collections::{HashMap, HashSet}; + +use gridcraft_core::date::{DateSystem, datetime_from_serial}; +use gridcraft_core::{CellRef, RangeRef, Value, split_sheet}; +use gridcraft_numfmt::{NumberFormat, format_value}; + +use crate::{PivotDateGroup, Workbook}; + +const INVALID: &str = "The reference isn't valid."; + +/// Month names of grouped dates' items. +pub const MONTHS: [&str; 12] = ["Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"]; + +/// The sheet and range (header row included) a source names: a table (without its totals row), +/// a range (whole columns or rows cut to the sheet's used range; one without a sheet is on +/// `default_sheet`), or a defined name holding either, followed a few levels deep. +pub fn resolve(wb: &Workbook, text: &str, default_sheet: usize) -> Result<(usize, RangeRef), String> { + resolve_inner(wb, text, default_sheet, 0) +} + +fn resolve_inner(wb: &Workbook, text: &str, default_sheet: usize, depth: u32) -> Result<(usize, RangeRef), String> { + let t = text.trim().trim_start_matches('=').trim(); + if t.is_empty() || depth > 4 { + return Err(INVALID.into()); + } + if let Some((si, ti)) = wb.table(t) + && let Some(tb) = wb.sheet(si).and_then(|s| s.tables.get(ti)) + { + if !tb.header_row { + return Err("The table needs a header row to be used as a PivotTable source.".into()); + } + let end_row = tb.range.end.row.saturating_sub(tb.totals_row as u32).max(tb.range.start.row); + return Ok((si, RangeRef::new(tb.range.start, CellRef::new(end_row, tb.range.end.col)))); + } + let (sheet, body) = split_sheet(t); + if let Some(r) = RangeRef::parse(body) { + let si = match sheet { + Some(n) => wb.sheet_index(&n).ok_or_else(|| INVALID.to_string())?, + None => default_sheet, + }; + let sh = wb.sheet(si).ok_or_else(|| INVALID.to_string())?; + let r = if r.is_full_cols() || r.is_full_rows() { + let used = sh.used_range().ok_or_else(|| INVALID.to_string())?; + r.intersection(&used).ok_or_else(|| INVALID.to_string())? + } else { + r + }; + return Ok((si, r)); + } + if sheet.is_none() + && let Some(n) = wb.name(t, default_sheet) + { + let f = n.formula.clone(); + return resolve_inner(wb, &f, default_sheet, depth + 1); + } + Err(INVALID.into()) +} + +/// The number format of a source column's items: its first record's. +pub fn column_format(wb: &Workbook, sheet: usize, range: RangeRef, col: u32) -> String { + match wb.sheet(sheet) { + Some(sh) if range.end.row > range.start.row => { + wb.styles.get(sh.style_id(CellRef::new(range.start.row + 1, col))).num_fmt.as_str().to_string() + } + _ => "General".into(), + } +} + +/// The source's column names: each header as it shows, trimmed, with a repeated one numbered +/// (`Amount`, `Amount2`). An empty header is an error, as a PivotTable needs labelled columns. +pub fn headers(wb: &Workbook, sheet: usize, range: RangeRef) -> Result, String> { + let sh = wb.sheet(sheet).ok_or(INVALID)?; + let mut out = Vec::new(); + // Names taken so far (lower-case), and the next suffix to try for each repeated header, so + // that thousands of equal headers don't take cubic time. + let mut taken: HashSet = HashSet::new(); + let mut next_suffix: HashMap = HashMap::new(); + for c in range.start.col..=range.end.col { + let at = CellRef::new(range.start.row, c); + let fmt = NumberFormat::parse(wb.styles.get(sh.style_id(at)).num_fmt.as_str()); + let h = format_value(&sh.value(at).scalar(), &fmt, wb.date_system).plain().trim().to_string(); + if h.is_empty() { + return Err( + "The PivotTable field name is not valid. To create a PivotTable, you must use data that is organized as a list with labeled columns." + .into(), + ); + } + let key = h.to_ascii_lowercase(); + let mut n = h.clone(); + let mut k = next_suffix.get(&key).copied().unwrap_or(2); + while taken.contains(&n.to_ascii_lowercase()) { + n = format!("{h}{k}"); + k += 1; + } + next_suffix.insert(key, k); + taken.insert(n.to_ascii_lowercase()); + out.push(n); + } + Ok(out) +} + +/// A date serial's item under a date grouping: its label and sort key. +pub fn date_group_item(sys: DateSystem, n: f64, group: PivotDateGroup) -> Option<(String, f64)> { + if group == PivotDateGroup::None { + return None; + } + let dt = datetime_from_serial(sys, n)?; + let mon = MONTHS.get(dt.month.saturating_sub(1) as usize).copied().unwrap_or("Jan"); + Some(match group { + PivotDateGroup::Years => (dt.year.to_string(), dt.year as f64), + PivotDateGroup::Quarters => { + let q = (dt.month.saturating_sub(1)) / 3 + 1; + (format!("Qtr{q}"), q as f64) + } + PivotDateGroup::Months => (mon.to_string(), dt.month as f64), + _ => (format!("{}-{mon}", dt.day), (dt.month * 100 + dt.day) as f64), + }) +} + +/// The labels a source column's items show: a grouped date's group, a number in the column's +/// format, `(blank)` for an empty cell, else the value's text. +pub struct ItemLabels { + fmt: NumberFormat, + group: PivotDateGroup, + sys: DateSystem, +} + +impl ItemLabels { + /// The labels of column `col` of source `range` on `sheet`, under `group`. + pub fn new(wb: &Workbook, sheet: usize, range: RangeRef, col: u32, group: PivotDateGroup) -> ItemLabels { + ItemLabels { fmt: NumberFormat::parse(&column_format(wb, sheet, range, col)), group, sys: wb.date_system } + } + + pub fn label(&self, v: &Value) -> String { + item_label(v, self.group, &self.fmt, self.sys) + } +} + +/// The label an item shows (see [`ItemLabels`]), its column's format `fmt` parsed. +pub fn item_label(v: &Value, group: PivotDateGroup, fmt: &NumberFormat, sys: DateSystem) -> String { + match v { + Value::Empty => "(blank)".into(), + Value::Number(n) => match date_group_item(sys, *n, group) { + Some((label, _)) => label, + None => format_value(v, fmt, sys).plain(), + }, + Value::Text(t) => t.to_string(), + Value::Bool(b) => (if *b { "TRUE" } else { "FALSE" }).into(), + Value::Error(e) => e.as_str().into(), + Value::Array(_) => item_label(&v.scalar(), group, fmt, sys), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{NumFmt, Style}; + + fn book(rows: &[&[Value]]) -> Workbook { + let mut wb = Workbook::new(); + if let Some(sh) = wb.sheet_mut(0) { + for (r, row) in rows.iter().enumerate() { + for (c, v) in row.iter().enumerate() { + sh.set_value(CellRef::new(r as u32, c as u32), v.clone()); + } + } + } + wb + } + + fn set_format(wb: &mut Workbook, at: &str, code: &str) { + let id = wb.styles.intern(Style { num_fmt: NumFmt::new(code), ..Style::default() }); + let at = CellRef::parse(at).unwrap(); + wb.sheet_mut(0).unwrap().set_style(at, id); + } + + #[test] + fn headers_are_trimmed_numbered_and_formatted() { + let mut wb = book(&[&[Value::text(" Amount "), Value::text("amount"), Value::Number(5.0), Value::text("Amount2")]]); + set_format(&mut wb, "C1", "0.0* "); + let r = RangeRef::parse("A1:D1").unwrap(); + assert_eq!(headers(&wb, 0, r).unwrap(), ["Amount", "amount2", "5.0", "Amount22"]); + let empty = book(&[&[Value::text("A"), Value::text(" ")]]); + assert!(headers(&empty, 0, RangeRef::parse("A1:B1").unwrap()).is_err()); + assert!(headers(&wb, 7, r).is_err()); + } + + #[test] + fn item_labels_follow_the_column_format_and_grouping() { + let mut wb = book(&[&[Value::text("When"), Value::text("Cost")], &[Value::Number(46037.0), Value::Number(3.5)]]); + set_format(&mut wb, "B2", "$0.00"); + let r = RangeRef::parse("A1:B2").unwrap(); + assert_eq!(column_format(&wb, 0, r, 1), "$0.00"); + assert_eq!(column_format(&wb, 0, RangeRef::parse("A1:B1").unwrap(), 1), "General"); + let cost = ItemLabels::new(&wb, 0, r, 1, PivotDateGroup::None); + assert_eq!(cost.label(&Value::Number(3.5)), "$3.50"); + assert_eq!(cost.label(&Value::Empty), "(blank)"); + assert_eq!(cost.label(&Value::Bool(true)), "TRUE"); + assert_eq!(cost.label(&Value::text("x")), "x"); + // 46037 is 15 Jan 2026. + let when = ItemLabels::new(&wb, 0, r, 0, PivotDateGroup::Quarters); + assert_eq!(when.label(&Value::Number(46037.0)), "Qtr1"); + assert_eq!(date_group_item(wb.date_system, 46037.0, PivotDateGroup::Months), Some(("Jan".into(), 1.0))); + assert_eq!(date_group_item(wb.date_system, 46037.0, PivotDateGroup::Days), Some(("15-Jan".into(), 115.0))); + assert_eq!(date_group_item(wb.date_system, 46037.0, PivotDateGroup::None), None); + assert_eq!(date_group_item(wb.date_system, -1.0, PivotDateGroup::Years), None); + } + + #[test] + fn resolve_follows_tables_ranges_and_names() { + let mut wb = book(&[&[Value::text("A")], &[Value::Number(1.0)], &[Value::Number(2.0)]]); + assert_eq!(resolve(&wb, "=Sheet1!A1:A3", 0).unwrap(), (0, RangeRef::parse("A1:A3").unwrap())); + assert_eq!(resolve(&wb, "A:A", 0).unwrap(), (0, RangeRef::parse("A1:A3").unwrap())); + assert!(resolve(&wb, "Nope!A1:A3", 0).is_err()); + assert!(resolve(&wb, "", 0).is_err()); + // A name naming itself stops after a few levels. + wb.names.push(crate::DefinedName { name: "Loop".into(), scope: None, formula: "Loop".into(), comment: String::new(), hidden: false }); + assert!(resolve(&wb, "Loop", 0).is_err()); + } +} diff --git a/crates/numfmt/src/lib.rs b/crates/numfmt/src/lib.rs index 52cada2c75..da7e61977a 100644 --- a/crates/numfmt/src/lib.rs +++ b/crates/numfmt/src/lib.rs @@ -87,6 +87,21 @@ pub struct Formatted { pub numeric: bool, } +impl Formatted { + /// The text as it reads at no particular width: a repeat fill (`*x`) shows as one space. + /// What text exports and labels use. + pub fn plain(self) -> String { + let mut t = self.text; + if let Some((_, pos)) = self.fill + && pos <= t.len() + && t.is_char_boundary(pos) + { + t.insert(pos, ' '); + } + t + } +} + #[derive(Debug)] struct Inner { code: String, diff --git a/crates/numfmt/src/tests.rs b/crates/numfmt/src/tests.rs index 4b2ee089d9..bfe76b7497 100644 --- a/crates/numfmt/src/tests.rs +++ b/crates/numfmt/src/tests.rs @@ -901,3 +901,10 @@ fn closed_brackets_still_hold_their_semicolons() { assert_eq!(f("0\\[0", 5.0), "0[5"); assert_eq!(f("\"[\"0;0", 5.0), "[5"); } + +#[test] +fn plain_shows_a_repeat_fill_as_one_space() { + assert_eq!(full("$* #,##0", Value::Number(1234.0)).plain(), "$ 1,234"); + assert_eq!(full("0.0", Value::Number(2.0)).plain(), "2.0"); + assert_eq!(full("@*-", Value::from("ab")).plain(), "ab "); +} diff --git a/docs/architecture.md b/docs/architecture.md index 68639541ea..c0f11d8120 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1,6 +1,6 @@ # Architecture -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-11-02 · **Change:** minor (Python mode in the formula bar; earlier: Python cells as data; earlier: equations: the `math` crate and the layout cache; earlier: recently used functions kept in the preferences; Function Library module; accent icons; earlier: the hidden names that hold macros) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-11-11 · **Change:** minor (pivot sources read in the model; earlier: Python mode in the formula bar; earlier: Python cells as data; earlier: equations: the `math` crate and the layout cache; earlier: recently used functions kept in the preferences; Function Library module; accent icons; earlier: the hidden names that hold macros) · **Target:** Microsoft Excel (Microsoft 365) How GridCraft is built today. Rules for contributors are in [`AGENTS.md`](../AGENTS.md). @@ -17,7 +17,7 @@ including tests, 2026-10-10. | L1 | `formula` | ~1.7k | Tokenizer, parser, AST, printer (A1 and R1C1), reference adjustment for copy, fill and structural edits | | L1 | `functions` | ~13.4k | The function library: `FnSpec` registry (name, arity, category, scalar args, signature, description) and pure implementations; automatic array lifting | | L1 | `math` | ~1.1k | Equations: LaTeX laid out by RaTeX (MIT, pinned) into plain glyph, rule and path data, and LaTeX to Office Math; every entry point capped and panic-guarded | -| L2 | `model` | ~2.2k | Workbook, sheets, copy-on-write cell store (64-row bands behind `Arc`), interned styles, names, tables, CF, validation, comments, charts, shapes, images, sparklines, print settings, protection | +| L2 | `model` | ~2.2k | Workbook, sheets, copy-on-write cell store (64-row bands behind `Arc`), interned styles, names, tables, CF, validation, comments, charts, shapes, images, sparklines, print settings, protection; a PivotTable's source as reports and file writers read it (`pivot_source`: range, column names, item labels) | | L2 | `calc` | ~2.9k | Evaluator (references, names, tables, LET/LAMBDA, special forms like OFFSET/INDIRECT/SUBTOTAL) and dependency-graph recalculation with dynamic arrays, spills and cycle detection; single-threaded | | L2 | `xlsx` | ~10.7k | XLSX read/write (transitional and strict read), CSV/TSV, XLSB and ODS data import, PivotTable caches, DrawingML charts and drawings; byte slices only (WASM-safe); lenient reader that reports warnings | | L2 | `pdf` | ~1k | Small PDF 1.7 writer (14 standard fonts, vectors, images) for print and export | @@ -236,6 +236,7 @@ rpm, tarball), FreeBSD, and web (WASM; WebGPU or WebGL2). Release workflows in | Date | Change | Summary | |---|---|---| +| 2026-11-11 | minor | `model::pivot_source` resolves a PivotTable's source and gives its column names and item labels; the engine's reports use it (file writers will too), and the model depends on `numfmt` (L2 → L0) for the labels; `numfmt::Formatted::plain` is the text at no width | | 2026-11-02 | minor | Python mode in the formula bar: Python mode on the shared editor, `code_field` and its stop keys; drafts follow their sheet by `model::SheetUid` | | 2026-11-02 | minor | Python cells as data: `model::PyCode`, the file forms, calc laying out kept results, the limits on open, and what copying, filling and sorting do to `xl()` references; edits that would push code past the limit are refused | | 2026-11-01 | trivial | Equations: the layout cache answers for a source over the cap by its length alone, without hashing it | From 38039f87c4f26bf7c87a610f4df9439aff3460f4 Mon Sep 17 00:00:00 2001 From: alehel Date: Wed, 11 Nov 2026 10:33:19 +0000 Subject: [PATCH 2/4] pivot: review fixes for the shared source module Number item labels keep the report's text with a repeat fill left out (as before this branch), so stored page selections such as " $120 " under an accounting format still match. item_label reads an array's first element once and calls a nested array (blank), as the item's value and key do, without recursing. MONTHS is private, and ItemLabels is left out until the file writer that uses it lands. --- crates/engine/src/cmd/pivot/tests.rs | 12 +++++++ crates/engine/src/pivot/items.rs | 28 +++++++++++++-- crates/model/src/pivot_source.rs | 54 ++++++++++++---------------- docs/architecture.md | 2 +- 4 files changed, 62 insertions(+), 34 deletions(-) diff --git a/crates/engine/src/cmd/pivot/tests.rs b/crates/engine/src/cmd/pivot/tests.rs index 88e5d2ed10..fb082c52b0 100644 --- a/crates/engine/src/cmd/pivot/tests.rs +++ b/crates/engine/src/cmd/pivot/tests.rs @@ -1130,3 +1130,15 @@ fn headers_read_values_while_formulas_show() { let r = s.execute("insert.pivotTable", json!({"rows": ["Region"], "values": ["Sales"]})).unwrap(); assert_eq!(r["fields"][0], "Region"); } + +#[test] +fn accounting_items_keep_their_labels_without_the_fill() { + let mut s = sales(); + s.execute("home.numberFormat", json!({"range": "E2:E13", "code": "_($* #,##0_);_($* (#,##0);_($* \"-\"_);_(@_)"})).unwrap(); + s.execute("insert.pivotTable", json!({"rows": ["Region"], "values": ["Units"], "filters": ["Sales"]})).unwrap(); + // The page item is labelled without a space where the fill is, and a selection stored by + // that label (as files and earlier versions hold it) still matches. + s.execute("pivot.filter", json!({"field": "Sales", "selected": [" $120 "]})).unwrap(); + assert_eq!(text(&s, "B1"), " $120 "); + assert_eq!(grand_total(&s, 1), Value::Number(20.0)); +} diff --git a/crates/engine/src/pivot/items.rs b/crates/engine/src/pivot/items.rs index a3394e70ec..d11c2029df 100644 --- a/crates/engine/src/pivot/items.rs +++ b/crates/engine/src/pivot/items.rs @@ -35,9 +35,10 @@ fn make_item(wb: &Workbook, v: &Value, group: PivotDateGroup, fmt: &str, first: first, formatted, }; - let v = &v.scalar(); - // The label is the model's, which file writers list items by too. + // The label is the model's, which file writers list items by too. It takes the value as the + // cell holds it and reads an array's first element the same way as the match below. let label = gridcraft_model::pivot_source::item_label(v, group, &crate::display::number_format(fmt), wb.date_system); + let v = &v.scalar(); match v { Value::Empty => ("b".into(), it(Value::text("(blank)"), label, 4, 0.0, false)), Value::Number(n) => match gridcraft_model::pivot_source::date_group_item(wb.date_system, *n, group) { @@ -253,3 +254,26 @@ pub(super) fn collect_siblings(nodes: &[Node], prefix: &mut Vec, out: &mut prefix.pop(); } } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use gridcraft_core::Array; + + use super::*; + + #[test] + fn array_items_agree_in_value_key_and_label() { + let wb = Workbook::new(); + let one = |v: Value| Value::Array(Arc::new(Array::scalar(v))); + // An array is its first element. + let (key, item) = make_item(&wb, &one(Value::Number(5.0)), PivotDateGroup::None, "General", 0); + assert_eq!(key, format!("n{}", 5f64.to_bits())); + assert_eq!((item.label.as_str(), &item.value), ("5", &Value::Number(5.0))); + // An array inside that is no value an item can hold: blank throughout. + let (key, item) = make_item(&wb, &one(one(Value::Number(5.0))), PivotDateGroup::None, "General", 0); + assert_eq!(key, "b"); + assert_eq!((item.label.as_str(), &item.value), ("(blank)", &Value::text("(blank)"))); + } +} diff --git a/crates/model/src/pivot_source.rs b/crates/model/src/pivot_source.rs index eaf53b39a6..4ffdde73de 100644 --- a/crates/model/src/pivot_source.rs +++ b/crates/model/src/pivot_source.rs @@ -13,7 +13,7 @@ use crate::{PivotDateGroup, Workbook}; const INVALID: &str = "The reference isn't valid."; /// Month names of grouped dates' items. -pub const MONTHS: [&str; 12] = ["Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"]; +const MONTHS: [&str; 12] = ["Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"]; /// The sheet and range (header row included) a source names: a table (without its totals row), /// a range (whole columns or rows cut to the sheet's used range; one without a sheet is on @@ -121,37 +121,21 @@ pub fn date_group_item(sys: DateSystem, n: f64, group: PivotDateGroup) -> Option }) } -/// The labels a source column's items show: a grouped date's group, a number in the column's -/// format, `(blank)` for an empty cell, else the value's text. -pub struct ItemLabels { - fmt: NumberFormat, - group: PivotDateGroup, - sys: DateSystem, -} - -impl ItemLabels { - /// The labels of column `col` of source `range` on `sheet`, under `group`. - pub fn new(wb: &Workbook, sheet: usize, range: RangeRef, col: u32, group: PivotDateGroup) -> ItemLabels { - ItemLabels { fmt: NumberFormat::parse(&column_format(wb, sheet, range, col)), group, sys: wb.date_system } - } - - pub fn label(&self, v: &Value) -> String { - item_label(v, self.group, &self.fmt, self.sys) - } -} - -/// The label an item shows (see [`ItemLabels`]), its column's format `fmt` parsed. +/// The label an item shows: a grouped date's group, a number in its column's format `fmt` (a +/// repeat fill left out, as the report has always shown items), `(blank)` for an empty cell, else +/// the value's text. An array stands for its first element; an array nested in that is no value +/// an item can hold, so it is `(blank)` (no recursion). pub fn item_label(v: &Value, group: PivotDateGroup, fmt: &NumberFormat, sys: DateSystem) -> String { + let v = &v.scalar(); match v { - Value::Empty => "(blank)".into(), + Value::Empty | Value::Array(_) => "(blank)".into(), Value::Number(n) => match date_group_item(sys, *n, group) { Some((label, _)) => label, - None => format_value(v, fmt, sys).plain(), + None => format_value(v, fmt, sys).text, }, Value::Text(t) => t.to_string(), Value::Bool(b) => (if *b { "TRUE" } else { "FALSE" }).into(), Value::Error(e) => e.as_str().into(), - Value::Array(_) => item_label(&v.scalar(), group, fmt, sys), } } @@ -196,14 +180,22 @@ mod tests { let r = RangeRef::parse("A1:B2").unwrap(); assert_eq!(column_format(&wb, 0, r, 1), "$0.00"); assert_eq!(column_format(&wb, 0, RangeRef::parse("A1:B1").unwrap(), 1), "General"); - let cost = ItemLabels::new(&wb, 0, r, 1, PivotDateGroup::None); - assert_eq!(cost.label(&Value::Number(3.5)), "$3.50"); - assert_eq!(cost.label(&Value::Empty), "(blank)"); - assert_eq!(cost.label(&Value::Bool(true)), "TRUE"); - assert_eq!(cost.label(&Value::text("x")), "x"); + let fmt = NumberFormat::parse(&column_format(&wb, 0, r, 1)); + let sys = wb.date_system; + let cost = |v: &Value| item_label(v, PivotDateGroup::None, &fmt, sys); + assert_eq!(cost(&Value::Number(3.5)), "$3.50"); + assert_eq!(cost(&Value::Empty), "(blank)"); + assert_eq!(cost(&Value::Bool(true)), "TRUE"); + assert_eq!(cost(&Value::text("x")), "x"); // 46037 is 15 Jan 2026. - let when = ItemLabels::new(&wb, 0, r, 0, PivotDateGroup::Quarters); - assert_eq!(when.label(&Value::Number(46037.0)), "Qtr1"); + let general = NumberFormat::parse("General"); + assert_eq!(item_label(&Value::Number(46037.0), PivotDateGroup::Quarters, &general, sys), "Qtr1"); + // A repeat fill is left out of a label, as the report has always shown it. + assert_eq!(item_label(&Value::Number(1234.0), PivotDateGroup::None, &NumberFormat::parse("$* #,##0"), sys), "$1,234"); + // An array is its first element; an array inside that is no item value. + let one = |v: Value| Value::Array(std::sync::Arc::new(gridcraft_core::Array::scalar(v))); + assert_eq!(item_label(&one(Value::Number(5.0)), PivotDateGroup::None, &general, sys), "5"); + assert_eq!(item_label(&one(one(Value::Number(5.0))), PivotDateGroup::None, &general, sys), "(blank)"); assert_eq!(date_group_item(wb.date_system, 46037.0, PivotDateGroup::Months), Some(("Jan".into(), 1.0))); assert_eq!(date_group_item(wb.date_system, 46037.0, PivotDateGroup::Days), Some(("15-Jan".into(), 115.0))); assert_eq!(date_group_item(wb.date_system, 46037.0, PivotDateGroup::None), None); diff --git a/docs/architecture.md b/docs/architecture.md index c0f11d8120..c75d2502c6 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -236,7 +236,7 @@ rpm, tarball), FreeBSD, and web (WASM; WebGPU or WebGL2). Release workflows in | Date | Change | Summary | |---|---|---| -| 2026-11-11 | minor | `model::pivot_source` resolves a PivotTable's source and gives its column names and item labels; the engine's reports use it (file writers will too), and the model depends on `numfmt` (L2 → L0) for the labels; `numfmt::Formatted::plain` is the text at no width | +| 2026-11-11 | minor | `model::pivot_source` resolves a PivotTable's source and gives its column names and item labels (as the report has always shown them); the engine's reports use it (file writers will too), and the model depends on `numfmt` (L2 → L0) for the labels; `numfmt::Formatted::plain` is the text at no width | | 2026-11-02 | minor | Python mode in the formula bar: Python mode on the shared editor, `code_field` and its stop keys; drafts follow their sheet by `model::SheetUid` | | 2026-11-02 | minor | Python cells as data: `model::PyCode`, the file forms, calc laying out kept results, the limits on open, and what copying, filling and sorting do to `xl()` references; edits that would push code past the limit are refused | | 2026-11-01 | trivial | Equations: the layout cache answers for a source over the cap by its length alone, without hashing it | From 6adbc4e40c7b9973f7c0e2b7acc76c0170e9f984 Mon Sep 17 00:00:00 2001 From: alehel Date: Sat, 14 Nov 2026 01:55:41 +0100 Subject: [PATCH 3/4] pivot: item labels take an opaque ItemFormat; long codes are General unscanned Round 1 review fixes for the shared pivot source module. - item_label took a numfmt::NumberFormat, which a file writer in the xlsx crate cannot name (the layers check bars xlsx -> numfmt). It now takes pivot_source::ItemFormat, built from a code string. - make_item parsed the column format uncached for every record (text and blank ones too), so a multi-megabyte code from a file made a report take seconds per thousand records. It uses the engine's format cache again, and a code over 4 x the character limit is General without being copied or scanned, as the grid shows it. - headers parse each style's format once, not once per column, with the same cap. - A header holding an array inside an array is read one level deep, as an item is, instead of formatting the inner value. - The header unit test used a trailing fill that trims away, so it could not tell plain() from text; it now uses a leading one. - Formatted::plain's doc said labels use it; item labels keep text. --- crates/engine/src/pivot/items.rs | 2 +- crates/model/src/pivot_source.rs | 57 ++++++++++++++++++++++++++------ crates/numfmt/src/lib.rs | 2 +- 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/pivot/items.rs b/crates/engine/src/pivot/items.rs index 8421bff00b..62144b2fd9 100644 --- a/crates/engine/src/pivot/items.rs +++ b/crates/engine/src/pivot/items.rs @@ -37,7 +37,7 @@ fn make_item(wb: &Workbook, v: &Value, group: PivotDateGroup, fmt: &str, first: }; // The label is the model's, which file writers list items by too. It takes the value as the // cell holds it and reads an array's first element the same way as the match below. - let label = gridcraft_model::pivot_source::item_label(v, group, &crate::display::number_format(fmt), wb.date_system); + let label = gridcraft_model::pivot_source::item_label(v, group, &crate::display::parsed(fmt).into(), wb.date_system); let v = &v.scalar(); match v { Value::Empty => ("b".into(), it(Value::text("(blank)"), label, 4, 0.0, false)), diff --git a/crates/model/src/pivot_source.rs b/crates/model/src/pivot_source.rs index 4ffdde73de..0640472b04 100644 --- a/crates/model/src/pivot_source.rs +++ b/crates/model/src/pivot_source.rs @@ -6,7 +6,7 @@ use std::collections::{HashMap, HashSet}; use gridcraft_core::date::{DateSystem, datetime_from_serial}; use gridcraft_core::{CellRef, RangeRef, Value, split_sheet}; -use gridcraft_numfmt::{NumberFormat, format_value}; +use gridcraft_numfmt::{MAX_FORMAT_CODE_CHARS, NumberFormat, format_value}; use crate::{PivotDateGroup, Workbook}; @@ -15,6 +15,34 @@ const INVALID: &str = "The reference isn't valid."; /// Month names of grouped dates' items. const MONTHS: [&str; 12] = ["Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"]; +/// The number format a source column's items are labelled in, parsed once for the column. It is +/// opaque so that a file writer can label items without depending on `numfmt` itself. +#[derive(Clone, Debug)] +pub struct ItemFormat(NumberFormat); + +impl ItemFormat { + /// Parses a format code as [`column_format`] gives it. A code too long to be valid is + /// General without being scanned, as the grid shows it. + pub fn new(code: &str) -> Self { + Self(parse_code(code)) + } +} + +impl From for ItemFormat { + fn from(f: NumberFormat) -> Self { + Self(f) + } +} + +/// `NumberFormat::parse`, except that a code of more bytes than 4 × the character limit (so +/// certainly over it, and General) is not copied or scanned. +fn parse_code(code: &str) -> NumberFormat { + if code.len() > 4 * MAX_FORMAT_CODE_CHARS { + return NumberFormat::parse("General"); + } + NumberFormat::parse(code) +} + /// The sheet and range (header row included) a source names: a table (without its totals row), /// a range (whole columns or rows cut to the sheet's used range; one without a sheet is on /// `default_sheet`), or a defined name holding either, followed a few levels deep. @@ -79,10 +107,14 @@ pub fn headers(wb: &Workbook, sheet: usize, range: RangeRef) -> Result = HashSet::new(); let mut next_suffix: HashMap = HashMap::new(); + // Each style's format is parsed once, however many headers share it. + let mut formats = HashMap::new(); for c in range.start.col..=range.end.col { let at = CellRef::new(range.start.row, c); - let fmt = NumberFormat::parse(wb.styles.get(sh.style_id(at)).num_fmt.as_str()); - let h = format_value(&sh.value(at).scalar(), &fmt, wb.date_system).plain().trim().to_string(); + let style = sh.style_id(at); + let fmt = formats.entry(style).or_insert_with(|| parse_code(wb.styles.get(style).num_fmt.as_str())); + // `format_value` reads an array's first element, and no deeper, as items do. + let h = format_value(&sh.value(at), fmt, wb.date_system).plain().trim().to_string(); if h.is_empty() { return Err( "The PivotTable field name is not valid. To create a PivotTable, you must use data that is organized as a list with labeled columns." @@ -125,13 +157,13 @@ pub fn date_group_item(sys: DateSystem, n: f64, group: PivotDateGroup) -> Option /// repeat fill left out, as the report has always shown items), `(blank)` for an empty cell, else /// the value's text. An array stands for its first element; an array nested in that is no value /// an item can hold, so it is `(blank)` (no recursion). -pub fn item_label(v: &Value, group: PivotDateGroup, fmt: &NumberFormat, sys: DateSystem) -> String { +pub fn item_label(v: &Value, group: PivotDateGroup, fmt: &ItemFormat, sys: DateSystem) -> String { let v = &v.scalar(); match v { Value::Empty | Value::Array(_) => "(blank)".into(), Value::Number(n) => match date_group_item(sys, *n, group) { Some((label, _)) => label, - None => format_value(v, fmt, sys).text, + None => format_value(v, &fmt.0, sys).text, }, Value::Text(t) => t.to_string(), Value::Bool(b) => (if *b { "TRUE" } else { "FALSE" }).into(), @@ -165,9 +197,13 @@ mod tests { #[test] fn headers_are_trimmed_numbered_and_formatted() { let mut wb = book(&[&[Value::text(" Amount "), Value::text("amount"), Value::Number(5.0), Value::text("Amount2")]]); - set_format(&mut wb, "C1", "0.0* "); + set_format(&mut wb, "C1", "$* 0.0"); let r = RangeRef::parse("A1:D1").unwrap(); - assert_eq!(headers(&wb, 0, r).unwrap(), ["Amount", "amount2", "5.0", "Amount22"]); + // A header shows its repeat fill as one space, as the cell's text does. + assert_eq!(headers(&wb, 0, r).unwrap(), ["Amount", "amount2", "$ 5.0", "Amount22"]); + // A code too long to be valid is General. + set_format(&mut wb, "C1", &"0".repeat(5000)); + assert_eq!(headers(&wb, 0, r).unwrap()[2], "5"); let empty = book(&[&[Value::text("A"), Value::text(" ")]]); assert!(headers(&empty, 0, RangeRef::parse("A1:B1").unwrap()).is_err()); assert!(headers(&wb, 7, r).is_err()); @@ -180,7 +216,7 @@ mod tests { let r = RangeRef::parse("A1:B2").unwrap(); assert_eq!(column_format(&wb, 0, r, 1), "$0.00"); assert_eq!(column_format(&wb, 0, RangeRef::parse("A1:B1").unwrap(), 1), "General"); - let fmt = NumberFormat::parse(&column_format(&wb, 0, r, 1)); + let fmt = ItemFormat::new(&column_format(&wb, 0, r, 1)); let sys = wb.date_system; let cost = |v: &Value| item_label(v, PivotDateGroup::None, &fmt, sys); assert_eq!(cost(&Value::Number(3.5)), "$3.50"); @@ -188,10 +224,11 @@ mod tests { assert_eq!(cost(&Value::Bool(true)), "TRUE"); assert_eq!(cost(&Value::text("x")), "x"); // 46037 is 15 Jan 2026. - let general = NumberFormat::parse("General"); + let general = ItemFormat::new("General"); assert_eq!(item_label(&Value::Number(46037.0), PivotDateGroup::Quarters, &general, sys), "Qtr1"); // A repeat fill is left out of a label, as the report has always shown it. - assert_eq!(item_label(&Value::Number(1234.0), PivotDateGroup::None, &NumberFormat::parse("$* #,##0"), sys), "$1,234"); + assert_eq!(item_label(&Value::Number(1234.0), PivotDateGroup::None, &ItemFormat::new("$* #,##0"), sys), "$1,234"); + assert_eq!(item_label(&Value::Number(5.0), PivotDateGroup::None, &ItemFormat::new(&"0".repeat(5000)), sys), "5"); // An array is its first element; an array inside that is no item value. let one = |v: Value| Value::Array(std::sync::Arc::new(gridcraft_core::Array::scalar(v))); assert_eq!(item_label(&one(Value::Number(5.0)), PivotDateGroup::None, &general, sys), "5"); diff --git a/crates/numfmt/src/lib.rs b/crates/numfmt/src/lib.rs index 99a1c05dfd..4a15cdfc0c 100644 --- a/crates/numfmt/src/lib.rs +++ b/crates/numfmt/src/lib.rs @@ -88,7 +88,7 @@ pub struct Formatted { impl Formatted { /// The text as it reads at no particular width: a repeat fill (`*x`) shows as one space. - /// What text exports and labels use. + /// What text exports and PivotTable headers use (PivotTable items keep `text`). pub fn plain(self) -> String { let mut t = self.text; if let Some((_, pos)) = self.fill From 2891981ee5724fc7a630e270ae95bf5325ef4fb8 Mon Sep 17 00:00:00 2001 From: alehel Date: Sat, 14 Nov 2026 02:12:12 +0100 Subject: [PATCH 4/4] pivot: parse each field's item format once per report Round 2 review fixes, correcting round 1's make_item change. - Round 1 had make_item look its column's format up in the format cache for every record: a global lock and a hash of the code per record, text and blank ones included, which main never paid for them. The report now builds each filter, row and column field's ItemFormat once, and distinct_labels does the same for its column; FieldItems::intern and make_item take the parsed format. - item_label's doc says callers pass each record as its cell's first element, as the report does; the engine comment said the opposite. - A test pins that a header is read one array level deep, as items are. --- crates/engine/src/pivot/compute.rs | 21 +++++++++++++-------- crates/engine/src/pivot/items.rs | 28 ++++++++++++++++++---------- crates/model/src/pivot_source.rs | 9 ++++++++- 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/crates/engine/src/pivot/compute.rs b/crates/engine/src/pivot/compute.rs index 534bb33328..204ae1c9e7 100644 --- a/crates/engine/src/pivot/compute.rs +++ b/crates/engine/src/pivot/compute.rs @@ -1,4 +1,6 @@ -use super::items::{Acc, FieldItems, build_tree, collect_siblings, is_collapsed, item_label}; +use gridcraft_model::pivot_source::{ItemFormat, item_label}; + +use super::items::{Acc, FieldItems, build_tree, collect_siblings, is_collapsed, item_format}; use super::output::{ AggMap, CellKind, ColKind, ColSpec, Ctx, Grid, Line, LineKind, Output, RowKind, ValSrc, caption, data_cols_exceed_sheet, flatten_cols, }; @@ -78,11 +80,12 @@ struct AggregateInput<'a> { vals: &'a [(ValSrc, &'a PivotValue)], extra: &'a [usize], filters: &'a [(usize, &'a PivotFilter, PivotDateGroup, bool)], - filter_fmts: &'a [String], + /// Each filter's, row field's and column field's item format, parsed once for the report. + filter_fmts: &'a [ItemFormat], /// Each filter's selected labels, ASCII-lowercased, so a record's test is one probe. filter_sets: &'a [Option>], - row_fmts: &'a [String], - col_fmts: &'a [String], + row_fmts: &'a [ItemFormat], + col_fmts: &'a [ItemFormat], empty: bool, } @@ -109,7 +112,7 @@ fn aggregate_record( ) -> Result<(), String> { for (((c, _, group, _), fmt), selected) in input.filters.iter().zip(input.filter_fmts).zip(input.filter_sets) { if let Some(selected) = selected { - let label = item_label(input.wb, rec.get(*c).unwrap_or(&Value::Empty), *group, fmt); + let label = item_label(rec.get(*c).unwrap_or(&Value::Empty), *group, fmt, input.wb.date_system); if !selected.contains(&label.to_ascii_lowercase()) { return Ok(()); } @@ -530,11 +533,13 @@ pub(super) fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut pro let row_levels = rows.len() + usize::from(on_rows); // ---- aggregate - let filter_fmts: Vec = filters.iter().map(|f| fmt_of(f.0)).collect(); + let filter_fmts: Vec = filters.iter().map(|f| item_format(&fmt_of(f.0))).collect(); let filter_sets: Vec>> = filters.iter().map(|(_, field, _, _)| field.selected.as_ref().map(|s| s.iter().map(|v| v.to_ascii_lowercase()).collect())).collect(); let row_fmts: Vec = rows.iter().map(|(c, _)| fmt_of(*c)).collect(); let col_fmts: Vec = cols.iter().map(|(c, _)| fmt_of(*c)).collect(); + let row_item_fmts: Vec = row_fmts.iter().map(|f| item_format(f)).collect(); + let col_item_fmts: Vec = col_fmts.iter().map(|f| item_format(f)).collect(); let aggregate = aggregate_source( AggregateInput { wb, @@ -547,8 +552,8 @@ pub(super) fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut pro filters: &filters, filter_fmts: &filter_fmts, filter_sets: &filter_sets, - row_fmts: &row_fmts, - col_fmts: &col_fmts, + row_fmts: &row_item_fmts, + col_fmts: &col_item_fmts, empty, }, &mut probe, diff --git a/crates/engine/src/pivot/items.rs b/crates/engine/src/pivot/items.rs index 62144b2fd9..8ff574a66a 100644 --- a/crates/engine/src/pivot/items.rs +++ b/crates/engine/src/pivot/items.rs @@ -1,3 +1,5 @@ +use gridcraft_model::pivot_source::ItemFormat; + use super::{CellError, HashMap, Ordering, PivotDateGroup, PivotField, PivotFunc, PivotSort, Source, Value, Workbook}; // ---------------------------------------------------------------- items @@ -25,7 +27,7 @@ fn cmp_items(a: &Item, b: &Item) -> Ordering { } /// (identity key, item) for a source value under a field's date grouping. -fn make_item(wb: &Workbook, v: &Value, group: PivotDateGroup, fmt: &str, first: usize) -> (String, Item) { +fn make_item(wb: &Workbook, v: &Value, group: PivotDateGroup, fmt: &ItemFormat, first: usize) -> (String, Item) { let it = |value: Value, label: String, class: u8, num: f64, formatted: bool| Item { text: label.to_lowercase(), value, @@ -35,9 +37,10 @@ fn make_item(wb: &Workbook, v: &Value, group: PivotDateGroup, fmt: &str, first: first, formatted, }; - // The label is the model's, which file writers list items by too. It takes the value as the - // cell holds it and reads an array's first element the same way as the match below. - let label = gridcraft_model::pivot_source::item_label(v, group, &crate::display::parsed(fmt).into(), wb.date_system); + // The label is the model's, which file writers list items by too. It takes the source record's + // value (already its cell's first element) and reads an array's first element the same way as + // the match below. + let label = gridcraft_model::pivot_source::item_label(v, group, fmt, wb.date_system); let v = &v.scalar(); match v { Value::Empty => ("b".into(), it(Value::text("(blank)"), label, 4, 0.0, false)), @@ -54,7 +57,7 @@ fn make_item(wb: &Workbook, v: &Value, group: PivotDateGroup, fmt: &str, first: } impl FieldItems { - pub(super) fn intern(&mut self, wb: &Workbook, v: &Value, group: PivotDateGroup, fmt: &str, first: usize) -> u32 { + pub(super) fn intern(&mut self, wb: &Workbook, v: &Value, group: PivotDateGroup, fmt: &ItemFormat, first: usize) -> u32 { let (key, item) = make_item(wb, v, group, fmt, first); if let Some(i) = self.index.get(&key) { return *i; @@ -74,16 +77,21 @@ impl FieldItems { /// The label of a source value under a date grouping (for filters and field lists). pub fn item_label(wb: &Workbook, v: &Value, group: PivotDateGroup, fmt: &str) -> String { - make_item(wb, v, group, fmt, 0).1.label + make_item(wb, v, group, &item_format(fmt), 0).1.label +} + +/// A source column's item format, parsed once for all its records (through the format cache). +pub(super) fn item_format(code: &str) -> ItemFormat { + crate::display::parsed(code).into() } /// Distinct item labels of a source column, sorted like the field list shows them. pub fn distinct_labels(wb: &Workbook, src: &Source, col: usize, group: PivotDateGroup, limit: usize) -> Vec { let mut fi = FieldItems::default(); - let fmt = src.formats.get(col).map(String::as_str).unwrap_or("General"); + let fmt = item_format(src.formats.get(col).map(String::as_str).unwrap_or("General")); for (i, r) in src.rows.iter().enumerate() { if let Some(v) = r.get(col) { - fi.intern(wb, v, group, fmt, i); + fi.intern(wb, v, group, &fmt, i); } if fi.items.len() > 100_000 { break; @@ -268,11 +276,11 @@ mod tests { let wb = Workbook::new(); let one = |v: Value| Value::Array(Arc::new(Array::scalar(v))); // An array is its first element. - let (key, item) = make_item(&wb, &one(Value::Number(5.0)), PivotDateGroup::None, "General", 0); + let (key, item) = make_item(&wb, &one(Value::Number(5.0)), PivotDateGroup::None, &item_format("General"), 0); assert_eq!(key, format!("n{}", 5f64.to_bits())); assert_eq!((item.label.as_str(), &item.value), ("5", &Value::Number(5.0))); // An array inside that is no value an item can hold: blank throughout. - let (key, item) = make_item(&wb, &one(one(Value::Number(5.0))), PivotDateGroup::None, "General", 0); + let (key, item) = make_item(&wb, &one(one(Value::Number(5.0))), PivotDateGroup::None, &item_format("General"), 0); assert_eq!(key, "b"); assert_eq!((item.label.as_str(), &item.value), ("(blank)", &Value::text("(blank)"))); } diff --git a/crates/model/src/pivot_source.rs b/crates/model/src/pivot_source.rs index 0640472b04..c685a01094 100644 --- a/crates/model/src/pivot_source.rs +++ b/crates/model/src/pivot_source.rs @@ -156,7 +156,8 @@ pub fn date_group_item(sys: DateSystem, n: f64, group: PivotDateGroup) -> Option /// The label an item shows: a grouped date's group, a number in its column's format `fmt` (a /// repeat fill left out, as the report has always shown items), `(blank)` for an empty cell, else /// the value's text. An array stands for its first element; an array nested in that is no value -/// an item can hold, so it is `(blank)` (no recursion). +/// an item can hold, so it is `(blank)` (no recursion). The report passes each record as its +/// cell's first element (`sheet.value(cell).scalar()`); a caller listing the same items does too. pub fn item_label(v: &Value, group: PivotDateGroup, fmt: &ItemFormat, sys: DateSystem) -> String { let v = &v.scalar(); match v { @@ -207,6 +208,12 @@ mod tests { let empty = book(&[&[Value::text("A"), Value::text(" ")]]); assert!(headers(&empty, 0, RangeRef::parse("A1:B1").unwrap()).is_err()); assert!(headers(&wb, 7, r).is_err()); + // A header is read one array level deep, as an item is: an array inside an array is no + // name. + let one = |v: Value| Value::Array(std::sync::Arc::new(gridcraft_core::Array::scalar(v))); + let nested = book(&[&[one(Value::text("A")), one(one(Value::text("B")))]]); + assert_eq!(headers(&nested, 0, RangeRef::parse("A1:A1").unwrap()).unwrap(), ["A"]); + assert!(headers(&nested, 0, RangeRef::parse("A1:B1").unwrap()).is_err()); } #[test]