Compare commits

..
Author SHA1 Message Date
Dylan M. Taylor 734b8c7755 address review: keep the original code in the public number_format for overlong formats 2026-11-04 01:41:46 +00:00
Dylan M. Taylor 6cdd6db150 engine: show overlong format codes as General without hashing them per cell
The display cache hashed the full format code on every lookup, so one
huge code shared by many cells cost its length for every cell drawn.
A code with more than 4 × 255 bytes can't be within Excel's 255
characters and is now formatted as General straight away.
2026-11-03 14:07:22 +00:00
8 changed files with 44 additions and 56 deletions
+39 -1
View File
@@ -8,8 +8,13 @@ use gridcraft_model::{Sheet, Workbook};
use gridcraft_numfmt::{Formatted, NumberFormat, format_value};
/// Parsed number formats are cached by code.
fn parsed(code: &str) -> NumberFormat {
pub(crate) fn parsed(code: &str) -> NumberFormat {
static CACHE: std::sync::LazyLock<Mutex<HashMap<String, NumberFormat>>> = std::sync::LazyLock::new(|| Mutex::new(HashMap::new()));
// A code of more bytes than 4 × the character limit can't be within it: it shows as General
// without being hashed or scanned again for every cell that shares it.
if code.len() > 4 * gridcraft_numfmt::MAX_FORMAT_CODE_CHARS {
return NumberFormat::parse("General");
}
let lock = || CACHE.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
if let Some(f) = lock().get(code) {
return f.clone();
@@ -24,7 +29,12 @@ fn parsed(code: &str) -> NumberFormat {
f
}
/// The parsed format of `code`; one longer than the character limit is a General format that keeps
/// the code it was made from. The engine itself formats through `parsed`, which doesn't copy it.
pub fn number_format(code: &str) -> NumberFormat {
if code.len() > 4 * gridcraft_numfmt::MAX_FORMAT_CODE_CHARS {
return NumberFormat::parse(code);
}
parsed(code)
}
@@ -226,3 +236,31 @@ pub fn stats(sheet: &Sheet, ranges: &[RangeRef]) -> Stats {
}
s
}
#[cfg(test)]
mod tests {
use super::*;
/// A format code far past Excel's 255 characters shows its values as General, for every cell
/// that shares it, without being hashed into the cache each time.
#[test]
fn an_overlong_code_formats_as_general() {
let wb = Workbook::new();
let code = format!("0.00{}", "\"x\"".repeat(400_000));
for _ in 0..1_000 {
assert_eq!(format(&Value::Number(1.5), &code, &wb).text, "1.5");
}
assert_eq!(format(&Value::Number(1.5), "0.00", &wb).text, "1.50");
}
/// The public `number_format` keeps the code of an overlong format; it is General, but not the
/// same format as a real `General`.
#[test]
fn number_format_keeps_an_overlong_code() {
let code = format!("0.00{}", "\"x\"".repeat(2_000));
let f = number_format(&code);
assert!(f.is_general());
assert_eq!(f.code(), code);
assert_ne!(f, number_format("General"));
}
}
+1 -1
View File
@@ -443,7 +443,7 @@ pub fn csv_values_bytes(wb: &Workbook) -> Result<Vec<u8>> {
let Some(sh) = wb.sheet(wb.active_sheet()) else { return Ok(Vec::new()) };
gridcraft_xlsx::write_csv(sh, wb.csv_delimiter.unwrap_or(b','), |c| match sh.value(c) {
gridcraft_core::Value::Number(n) if n.is_finite() => {
let shape = crate::display::number_format(wb.styles.get(sh.style_id(c)).num_fmt.as_str()).date_shape(n);
let shape = crate::display::parsed(wb.styles.get(sh.style_id(c)).num_fmt.as_str()).date_shape(n);
match shape.zip(gridcraft_core::date::datetime_from_serial(wb.date_system, n)) {
Some(((date, time), d)) => iso(&d, date, time),
None => n.to_string(),
+1 -1
View File
@@ -440,7 +440,7 @@ pub fn infer_input(wb: &Workbook, sheet: usize, r: RangeRef) -> NamedInput {
Some(match v {
Value::Empty => return None,
Value::Text(t) if t.is_empty() => return None,
Value::Number(_) if crate::display::number_format(wb.styles.get(sh.style_id(c)).num_fmt.as_str()).is_date() => ColType::Date,
Value::Number(_) if crate::display::parsed(wb.styles.get(sh.style_id(c)).num_fmt.as_str()).is_date() => ColType::Date,
Value::Number(_) => ColType::Number,
Value::Text(_) => ColType::Text,
Value::Bool(_) => ColType::Bool,
+1 -1
View File
@@ -335,7 +335,7 @@ fn check_input(wb: &Workbook, input: &gridcraft_model::NamedInput, place: Place)
gridcraft_model::ColType::Number => v.is_number(),
gridcraft_model::ColType::Text => v.is_text(),
gridcraft_model::ColType::Bool => matches!(v, Value::Bool(_)),
gridcraft_model::ColType::Date => v.is_number() && crate::display::number_format(&date()).is_date(),
gridcraft_model::ColType::Date => v.is_number() && crate::display::parsed(&date()).is_date(),
};
if !ok {
wrong.push((c, holds(&v, ty)));
+1 -1
View File
@@ -182,7 +182,7 @@ pub fn read_source(wb: &Workbook, text: &str, default_sheet: usize) -> Result<So
for (n, a) in numeric.iter_mut().zip(any.iter()) {
*n = *n && *a;
}
let dates = numeric.iter().zip(formats.iter()).map(|(n, f)| *n && crate::display::number_format(f).is_date()).collect();
let dates = numeric.iter().zip(formats.iter()).map(|(n, f)| *n && crate::display::parsed(f).is_date()).collect();
Ok(Source { sheet: si, range: r, headers, formats, numeric, dates, rows })
}
+1 -15
View File
@@ -332,14 +332,6 @@ impl NumberFormat {
None => Formatted { text: t.to_string(), color: None, fill: None, numeric: false },
}
}
/// [`Self::format_text`]'s text, or `None` once it passes `max_units` UTF-16 code units.
fn format_text_within(&self, t: &str, max_units: usize) -> Option<String> {
match self.inner.text_section.and_then(|i| self.inner.sections.get(i)) {
Some(sec) => render::render_text_within(sec, t, max_units).map(|out| out.text),
None => (t.encode_utf16().count() <= max_units).then(|| t.to_string()),
}
}
}
/// Formats a cell value for display.
@@ -433,10 +425,6 @@ impl NumberFormat {
/// The `TEXT()` worksheet function: formats a value with a format code. Numeric text is
/// converted to a number first; other text and booleans (as `TRUE`/`FALSE`) go through the text
/// section, or are returned as is.
/// Longest text a cell holds, in UTF-16 code units (Excel's 32,767 characters, which XLSB also
/// stores that way), and so the longest `TEXT` result.
const MAX_CELL_TEXT_CHARS: usize = 32_767;
pub fn text_function(v: &Value, code: &str, sys: DateSystem) -> Result<String, CellError> {
if code.is_empty() {
return Ok(String::new());
@@ -452,9 +440,7 @@ pub fn text_function(v: &Value, code: &str, sys: DateSystem) -> Result<String, C
Value::Bool(b) => return Ok(fmt.format_text(if b { "TRUE" } else { "FALSE" }).text),
Value::Text(t) => match gridcraft_core::parse::parse_number_text(&t) {
Some(n) => n,
// Every `@` repeats the text: a result past a cell's limit is `#VALUE!`, so nested
// TEXT calls can't multiply it further.
None => return fmt.format_text_within(&t, MAX_CELL_TEXT_CHARS).ok_or(CellError::Value),
None => return Ok(fmt.format_text(&t).text),
},
Value::Array(_) => return Err(CellError::Value),
};
-29
View File
@@ -45,35 +45,6 @@ pub(crate) fn render_text(sec: &Section, text: &str) -> Out {
out
}
/// [`render_text`], but `None` once the output passes `max_units` UTF-16 code units (a cell's
/// limit), checked before each piece is appended, so an oversized result is never built.
pub(crate) fn render_text_within(sec: &Section, text: &str, max_units: usize) -> Option<Out> {
let text_units = text.encode_utf16().count();
let mut out = Out::default();
let mut units = 0usize;
for t in &sec.toks {
match t {
Tok::At => {
units = units.saturating_add(text_units);
if units > max_units {
return None;
}
out.push(text);
}
Tok::Fill(c) => out.fill(*c),
other => {
let literal = other.literal_text();
units = units.saturating_add(literal.encode_utf16().count());
if units > max_units {
return None;
}
out.push(&literal);
}
}
}
Some(out)
}
/// Renders a number through a General section (literals around the General keyword).
pub(crate) fn render_general(sec: &Section, v: f64, width: usize) -> Out {
let mut out = Out::default();
-7
View File
@@ -396,13 +396,6 @@ fn text_fn() {
assert_eq!(text_function(&Value::Number(1234.5), "$#,##0.00", S), Ok("$1,234.50".into()));
assert_eq!(text_function(&Value::from("5"), "0.00", S), Ok("5.00".into()));
assert_eq!(text_function(&Value::from("abc"), "0.00", S), Ok("abc".into()));
// Each `@` repeats the text: up to a cell's 32,767 characters, then #VALUE!.
let at = "@".repeat(255);
assert_eq!(text_function(&Value::from("x".repeat(128).as_str()), &at, S).map(|s| s.len()), Ok(128 * 255));
assert_eq!(text_function(&Value::from("x".repeat(32_000).as_str()), &at, S), Err(CellError::Value));
// The limit counts UTF-16 units: 16,384 emoji are 32,768 of them, one too many.
assert_eq!(text_function(&Value::from("\u{1F600}".repeat(16_383).as_str()), "@", S).map(|s| s.chars().count()), Ok(16_383));
assert_eq!(text_function(&Value::from("\u{1F600}".repeat(16_384).as_str()), "@", S), Err(CellError::Value));
assert_eq!(text_function(&Value::Error(CellError::NA), "0", S), Err(CellError::NA));
assert_eq!(text_function(&Value::Number(46302.0), "yyyy", S), Ok("2026".into()));
assert_eq!(text_function(&Value::Number(0.5), "General", S), Ok("0.5".into()));