From bf9a202222963cf5febea36a3b64d9e772362e1a Mon Sep 17 00:00:00 2001 From: Richard Date: Mon, 12 Oct 2026 20:29:00 +1100 Subject: [PATCH 1/2] Compute the Top 10 filter threshold once per column, not once per row (#661) apply_filter cloned and sorted the column's numbers for every row it tested, O(n^2 log n). The threshold is now computed once per criterion with the other column stats. Adds a test for top/bottom items and percent. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Ut3hr3a13szpQdQ7gbYbfc --- crates/engine/src/cmd/data.rs | 27 ++++++++++++++++----------- crates/engine/src/tests.rs | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index e4aa3ba..0effb86 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -570,14 +570,23 @@ pub(crate) fn apply_filter(wb: &mut Workbook, sheet: usize) { let Some(af) = sh.autofilter.clone() else { return }; let r = af.range; let mut hide = Vec::new(); - // Precompute per-column numbers for top10/average. - let mut col_stats: std::collections::HashMap, f64)> = std::collections::HashMap::new(); + // Precompute per-column top10 threshold and average once, not per row. + let mut col_stats: std::collections::HashMap, f64)> = std::collections::HashMap::new(); for (off, c) in &af.criteria { if matches!(c, FilterCriterion::Top10 { .. } | FilterCriterion::AboveAverage(_)) { let col = r.start.col + off; - let nums: Vec = (r.start.row + 1..=r.end.row).filter_map(|row| sh.value(CellRef::new(row, col)).as_f64()).collect(); + let mut nums: Vec = (r.start.row + 1..=r.end.row).filter_map(|row| sh.value(CellRef::new(row, col)).as_f64()).collect(); let avg = if nums.is_empty() { 0.0 } else { nums.iter().sum::() / nums.len() as f64 }; - col_stats.insert(*off, (nums, avg)); + let thr = match c { + FilterCriterion::Top10 { bottom, percent, count } => { + nums.sort_by(|a, b| if *bottom { a.partial_cmp(b) } else { b.partial_cmp(a) }.unwrap_or(Ordering::Equal)); + let k = if *percent { ((nums.len() as f64) * (*count as f64) / 100.0).ceil() as usize } else { *count as usize }; + let k = k.clamp(1, nums.len().max(1)); + nums.get(k - 1).copied() + } + _ => None, + }; + col_stats.insert(*off, (thr, avg)); } } for row in r.start.row + 1..=r.end.row { @@ -604,13 +613,9 @@ pub(crate) fn apply_filter(wb: &mut Workbook, sheet: usize) { None => x, } } - FilterCriterion::Top10 { bottom, percent, count } => match (v.as_f64(), col_stats.get(off)) { - (Some(n), Some((nums, _))) => { - let mut sorted = nums.clone(); - sorted.sort_by(|a, b| if *bottom { a.partial_cmp(b) } else { b.partial_cmp(a) }.unwrap_or(Ordering::Equal)); - let k = if *percent { ((sorted.len() as f64) * (*count as f64) / 100.0).ceil() as usize } else { *count as usize }; - let k = k.clamp(1, sorted.len().max(1)); - let thr = sorted.get(k - 1).copied().unwrap_or(n); + FilterCriterion::Top10 { bottom, .. } => match (v.as_f64(), col_stats.get(off)) { + (Some(n), Some((thr, _))) => { + let thr = thr.unwrap_or(n); if *bottom { n <= thr } else { n >= thr } } _ => false, diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 33fdc11..2d12822 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -805,6 +805,24 @@ fn sort_and_filter() { assert_eq!(r["hiddenRows"], 1); } +/// Top 10 keeps the top or bottom N items or percent, ties at the threshold included. +#[test] +fn filter_top10_items_and_percent() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Score"], [5], [1], [9], [3], [9], [7], ["x"]]})).unwrap(); + s.execute("selection.set", json!({"cell": "A1"})).unwrap(); + s.execute("data.filter", json!({})).unwrap(); + // Top 2 items: 9, 9 (the "x" text row is hidden too). + let r = s.execute("data.filterBy", json!({"column": "A", "top": {"count": 2}})).unwrap(); + assert_eq!(r["hiddenRows"], 5); + // Bottom 2 items: 1, 3. + let r = s.execute("data.filterBy", json!({"column": "A", "top": {"count": 2, "bottom": true}})).unwrap(); + assert_eq!(r["hiddenRows"], 5); + // Top 50 percent of 6 numbers = 3 items; the threshold 7 keeps 9, 9, 7. + let r = s.execute("data.filterBy", json!({"column": "A", "top": {"count": 50, "percent": true}})).unwrap(); + assert_eq!(r["hiddenRows"], 4); +} + /// Turning the filter on with only the header row selected filters the data below it (#201). #[test] fn filter_on_a_selected_header_row_covers_the_data_below() { From ae54c84bc90cda1032df8b7af03d6c82b09f9d0a Mon Sep 17 00:00:00 2001 From: Richard Date: Tue, 13 Oct 2026 00:10:54 +1100 Subject: [PATCH 2/2] Key the cached Top 10 cutoff by criterion, not column (#661) Two Top 10 criteria on one column (possible in native JSON files) shared one cache entry, so the last cutoff overwrote the first. Reported by @derechopiano. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Ut3hr3a13szpQdQ7gbYbfc --- crates/engine/src/cmd/data.rs | 14 +++++++------- crates/engine/src/tests.rs | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 0effb86..f9b831c 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -570,9 +570,9 @@ pub(crate) fn apply_filter(wb: &mut Workbook, sheet: usize) { let Some(af) = sh.autofilter.clone() else { return }; let r = af.range; let mut hide = Vec::new(); - // Precompute per-column top10 threshold and average once, not per row. - let mut col_stats: std::collections::HashMap, f64)> = std::collections::HashMap::new(); - for (off, c) in &af.criteria { + // Precompute per-criterion top10 threshold and average once, not per row. + let mut col_stats: std::collections::HashMap, f64)> = std::collections::HashMap::new(); + for (i, (off, c)) in af.criteria.iter().enumerate() { if matches!(c, FilterCriterion::Top10 { .. } | FilterCriterion::AboveAverage(_)) { let col = r.start.col + off; let mut nums: Vec = (r.start.row + 1..=r.end.row).filter_map(|row| sh.value(CellRef::new(row, col)).as_f64()).collect(); @@ -586,12 +586,12 @@ pub(crate) fn apply_filter(wb: &mut Workbook, sheet: usize) { } _ => None, }; - col_stats.insert(*off, (thr, avg)); + col_stats.insert(i, (thr, avg)); } } for row in r.start.row + 1..=r.end.row { let mut visible = true; - for (off, crit) in &af.criteria { + for (i, (off, crit)) in af.criteria.iter().enumerate() { let c = CellRef::new(row, r.start.col + off); let v = sh.value(c); let text = crate::display::cell_text(wb, sh, c); @@ -613,14 +613,14 @@ pub(crate) fn apply_filter(wb: &mut Workbook, sheet: usize) { None => x, } } - FilterCriterion::Top10 { bottom, .. } => match (v.as_f64(), col_stats.get(off)) { + FilterCriterion::Top10 { bottom, .. } => match (v.as_f64(), col_stats.get(&i)) { (Some(n), Some((thr, _))) => { let thr = thr.unwrap_or(n); if *bottom { n <= thr } else { n >= thr } } _ => false, }, - FilterCriterion::AboveAverage(above) => match (v.as_f64(), col_stats.get(off)) { + FilterCriterion::AboveAverage(above) => match (v.as_f64(), col_stats.get(&i)) { (Some(n), Some((_, avg))) => { if *above { n > *avg diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 2d12822..8c8b08f 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -823,6 +823,28 @@ fn filter_top10_items_and_percent() { assert_eq!(r["hiddenRows"], 4); } +/// Two Top 10 criteria on one column (possible in a native JSON file) each keep their own +/// cutoff: Top 3 and Bottom 4 of 1..5 keep 3 and 4. +#[test] +fn filter_top10_two_criteria_on_one_column() { + use gridcraft_model::FilterCriterion; + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["N"], [1], [2], [3], [4], [5]]})).unwrap(); + s.execute("selection.set", json!({"cell": "A1"})).unwrap(); + s.execute("data.filter", json!({})).unwrap(); + let sh = std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb).sheet_mut(0).unwrap(); + if let Some(af) = sh.autofilter.as_mut() { + af.criteria = vec![ + (0, FilterCriterion::Top10 { bottom: false, percent: false, count: 3 }), + (0, FilterCriterion::Top10 { bottom: true, percent: false, count: 4 }), + ]; + } + s.execute("data.reapply", json!({})).unwrap(); + let hidden: Vec = s.active().unwrap().wb.sheet(0).unwrap().rows.iter().filter(|(_, r)| r.hidden).map(|(k, _)| *k).collect(); + // Rows are 0-based: values 1, 2 and 5 sit on rows 1, 2 and 5. + assert_eq!(hidden, vec![1, 2, 5]); +} + /// Turning the filter on with only the header row selected filters the data below it (#201). #[test] fn filter_on_a_selected_header_row_covers_the_data_below() {