[vangel76]

Fixes #2679

What. MEDIAN, PERCENTILE(.INC/.EXC), QUARTILE(.INC/.EXC), LARGE and SMALL now find the one or two order statistics they need with select_nth_unstable_by(f64::total_cmp) (O(n)) instead of sorting all the data (O(n log n)). Input that is already sorted (ascending or descending, e.g. a sorted column) is detected by one cheap pass and read directly, because the std sort handles such runs in O(n) and selection is slower there. MEDIAN with an even count takes the maximum of the lower partition for the lower middle value; PERCENTILE's interpolation takes the minimum of the upper partition as the next value (only computed when the fractional part is non-zero). PERCENTRANK and other sorted() callers are untouched.

Why. The sort was the dominant cost, and LARGE(range, SEQUENCE(300)) repeated it once per spilled cell.

Numbers (release, taskset on one core, perf stat -e instructions:u, setup subtracted, 4 identical runs each):

Call	Before	After
MEDIAN, 1M numbers	0.34 G instr	0.054 G (6x)
LARGE(1M numbers, 1)	0.34 G	0.034 G (10x)
PERCENTILE(1M numbers, 0.37)	0.34 G	0.058 G (6x)
300 x LARGE(100k numbers, k)	8.75 G	1.26 G (6.9x)

Adversarial inputs (same method, instructions per call, upstream/main vs this branch; core = aggregate_values(_k), formula = call with a range argument, LARGE/SMALL with k = {1,2,3,10} lifted):

Input	MEDIAN / PERCENTILE / LARGE core	MEDIAN formula	LARGE and SMALL, 4 k values
128 random	0.5x	0.5x	0.4x
10,000 sorted, reverse-sorted, all equal	1.03x	1.03x	1.03x
10,000 duplicate-heavy (10 distinct, random order)	0.6-0.7x	0.63x	0.44-0.51x
10,000 random	0.2x	0.22x	0.15-0.18x
1M random	0.14-0.17x	0.15x	0.11-0.12x
10,000 "organ pipe" (up, then down)	1.5x (MEDIAN), 1.1-1.2x (LARGE, PERCENTILE)	1.57x	0.7-1.0x

Before the sorted-input check, sorted/reverse/block-duplicate 10,000-value inputs were 1.2-1.8x main. The organ-pipe shape (instructions only; wall time at 10,000 was equal or lower) is the one remaining case where MEDIAN costs more than a full sort.

How verified. The new test selection_matches_full_sort_bit_for_bit keeps the old full-sort implementations as an oracle and compares results bit for bit (or same error) on randomised data: lengths 0 to 4096, many ties, -0.0/+0.0, NaN, infinities, k and percentile values out of range, fractional k, and both LARGE and SMALL. The test also covers already ascending, descending and nearly ascending data. cargo test -p gridcraft-functions passes (148 tests), cargo fmt --all --check and cargo clippy -p gridcraft-functions --all-targets -- -D warnings are clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Performance
  • Improved the efficiency of median, percentile, and rank-based statistical calculations, especially for larger datasets and inputs that are already sorted or nearly sorted.
  • Existing input validation, ranking behavior, and interpolation rules are preserved, so these calculations continue to return results according to the same rules. Empty median inputs and special floating-point values continue to be handled consistently.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: storytold/gridcraft
• Branch: main
• File: .coderabbit.yaml
• Commit: 146161b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 172e1520-3bbc-4551-88d2-0fff78e5f85f

Commits

Reviewing files that changed from the base of the PR and between a434efa and 8b330ac.

Files selected for processing: 2

• crates/functions/src/stat/descriptive.rs
• crates/functions/src/stat/tests.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

Walkthrough

Median, kth, and percentile calculations now use in-place selection or direct access for monotone inputs instead of fully sorting their inputs. Deterministic tests compare results and errors with full-sort reference implementations.

Changes: Statistical selection

Layer / File(s)	Summary
Select statistical ranks and interpolate percentiles crates/functions/src/stat/descriptive.rs	median, kth, and percentile interpolation use in-place selection or direct access for monotone inputs. Existing validation and result rules remain.
Check selection against full-sort references crates/functions/src/stat/tests.rs	Tests compare results bit-for-bit and errors exactly across varied input lengths, special floating-point values, ties, and boundary parameters.

Priority: Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Suggested reviewers: aiongg

Merge Risk: Minimal · up to 8b330

The statistical calculations use selection instead of full sorting. No merge-blocking issue was identified from the available evidence.

Architecture risk: Medium · up to 89087

The change affects 1 system.

Changed systems: crates

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — crates (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in crates/functions/src/stat.rs: median now selects the middle order statistic in place instead of sorting the vector; for even-length inputs it takes the maximum of the lower partition and returns its midpoint with the selected upper value. Empty-input handling is unchanged.
• observed — Modified behavior in crates/functions/src/stat.rs: kth now selects the requested order statistic in place rather than indexing a sorted copy. It retains the existing ceiling of k, invalid-rank #NUM! result, and largest-versus-smallest index mapping.
• observed — Modified behavior in crates/functions/src/stat.rs: interp now selects the lower ranked value and finds the next value as the minimum of the upper partition when interpolation is needed, replacing indexing into sorted data. The inclusive percentile path passes its vector directly to selection after the existing empty-input and k checks.
• observed — Modified behavior in crates/functions/src/stat.rs: The exclusive percentile path now passes its vector directly to interp, replacing the full sort; its existing k and position validation remains unchanged.

Reliability and maintainability

• inferred — Risk-relevant change factors for crates: blast_radius_1; direct_dependents_1

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly identifies the main performance change: replacing sorting with selection in MEDIAN, PERCENTILE, QUARTILE, LARGE, and SMALL.
Linked Issues check		Issue #2679 requires selection for MEDIAN, PERCENTILE.INC, PERCENTILE.EXC, QUARTILE.INC, QUARTILE.EXC, LARGE, and SMALL. The reviewed changes use select_nth_unstable_by with f64::total_cmp for uno…
Out of Scope Changes check		The changes stay within issue #2679. They modify order-statistic implementations and add focused oracle tests. The pub(super) visibility change supports those tests. PERCENTRANK and other full-order…

Full details: Linked Issues check

Explanation

Issue #2679 requires selection for MEDIAN, PERCENTILE.INC, PERCENTILE.EXC, QUARTILE.INC, QUARTILE.EXC, LARGE, and SMALL. The reviewed changes use select_nth_unstable_by with f64::total_cmp for unordered inputs and handle monotone inputs directly. The implementation uses the lower partition maximum for even-count MEDIAN and the upper partition minimum for percentile interpolation. QUARTILE functions use the percentile path. The added oracle test compares results with the former full-sort behavior across ties, signed zero, NaN, infinities, invalid parameters, and sorted data.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #2679. They modify order-statistic implementations and add focused oracle tests. The pub(super) visibility change supports those tests. PERCENTRANK and other full-order callers remain unchanged.

Full details: Docstring Coverage

Explanation

Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 13 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[vangel76]

This PR is one of five speed-ups from an audit of the functions and calc crates (issues #2679 and #2681 to #2685). Each one changes a different place; all are based on current main and none needs another to merge first.

PR	Fixes	Change	Measured (one pinned core, perf stat -e instructions:u unless noted)
#2686	#2679	MEDIAN, PERCENTILE, QUARTILE, LARGE, SMALL select instead of fully sorting	MEDIAN of 1M 0.34 G to 0.054 G (6x); LARGE(1M,1) 0.34 G to 0.034 G (10x); 300 x LARGE(100k,k) 8.75 G to 1.26 G (6.9x)
#2691	#2681	per-pass memo of (sum, count) for constant ranges; longer ranges extend shorter ones	20k rows of =A/SUM(A$1:A$20000) 20.2 s to 19 ms; 20k running totals 8.9 s to 16 ms; 1000 x SUM over 50k rows 2.0 s to 2.1 ms
#2693	#2685	rebuild and cells_changed share the formula AST instead of cloning it	rebuild of 100k IF formulas 1086 M to 747 M (-31 %)
#2694	#2683	spatial index for spill ranges	20k MAX(A:B) rows with 2000 spills 1160 ms to 77 ms; no-spill control unchanged
#2696	#2684	set of #SPILL! cells instead of walking all graph nodes and a 64x64 window	clearing one cell with 100k formulas 10 ms to 0; 20k-cell paste 20 ms to 10.5 ms

Results are identical in every case. #2686 compares against the old full-sort code bit for bit (lengths 0 to 4096, ties, -0.0, NaN, inf). #2691 compares against a plain fold, bit for bit, over full recalculations and 30 edit rounds and against the unmodified build on random workbooks with errors, text, spills and RANDBETWEEN. #2694 and #2696 compare byte-identical dumps of all values, spill and spill_ranges against the old build on about 2,000 and 1,500 random workbooks with overlapping and blocked spills, iterative calculation on and off, and edit batches of 1 to 76 cells. The measurements use synthetic workbooks.

Open PRs that touch the same code and will need a small merge: #2319 and #2252 (recalc.rs ranges, #2691), #1954, #2319 and #2483 (spills, #2694), #2653, #2639 and #2637 (rebuild, #2693). #2694 and #2696 only collide in the tests appended to tests.rs. The lookup copies (#2678) wait until #2036, #2037 and #2409 are merged.

🤖 Generated with Claude Code

[FredApps]

Reviewed the selection changes and surrounding callers at this head. I found no new concrete correctness defect: total_cmp defines the same ordering for selection and sorting, the even median uses the lower partition maximum, and interpolation uses the upper partition minimum only for a nonzero fractional rank. Existing input guards prevent invalid selection indices; QUARTILE delegates to the changed percentile helpers.

Local validation: a dependency-free Rust harness extracted the production helper bodies and the committed old-sort oracle test unchanged (only the error type/import scaffolding was supplied). selection_matches_full_sort_bit_for_bit passed: 1 passed, 0 failed. This tests the algorithms, not the complete function-dispatch path.

CI evidence: inspected the successful gate job log, including this oracle test; 2,525 tests passed, one timing-sensitive calc scaling test skipped, and all ten gate steps passed (including fmt, Clippy and wasm). These are CI results, not a local Cargo pass.

The reported instruction counts remain author measurements. They demonstrate the large synthetic order-statistic hot path; sorted/reverse-sorted inputs, small inputs, and whole-formula range collection can change the end-to-end speedup. I did not reproduce those benchmark numbers.

Local Cargo validation was blocked before compilation: the pinned egui source was unavailable offline, and the isolated crate build could not download missing crates because the TLS client credentials were unavailable. No security settings were changed. This is a COMMENT because local integration validation and independent performance reproduction remain incomplete.

[vangel76]

Merged current main into this branch. stat.rs was split into stat/ on main, so the merge conflicted. I re-applied the selection changes (median, kth, interp, percentile/quartile) by hand in stat/descriptive.rs and moved the old-vs-new oracle test to stat/tests.rs. The diff against main is again only those two files. fmt, clippy -D warnings and the gridcraft-functions stat tests pass, including the bit-for-bit selection_matches_full_sort test.

[Harrison-Booth]

Compared this branch with upstream main on one Windows machine, using release-mode gridcraft-functions builds. Each value is the median of five samples in microseconds. Core timings include cloning the input vector; formula timings include constructing the range argument and evaluating array-lifted LARGE with k=[1,2,3,10]. Inputs: 128 small; 10,000 sorted, reverse-sorted, and duplicate-heavy; 1,000,000 values.

Input	Core MEDIAN main→branch	Core LARGE main→branch	Core PERCENTILE.INC main→branch	Range + array LARGE main→branch
Small	2.155→0.975	2.144→0.831	2.164→0.757	20.871→24.412
Sorted	87.100→42.910	52.910→30.410	69.600→29.340	1350.650→1678.120
Reverse	78.390→81.790	73.610→76.800	52.650→99.680	1220.580→1845.500
Duplicate-heavy	71.180→84.570	67.210→96.760	139.750→108.560	1357.570→1934.050
1,000,000	42096.500→6601.000	40155.700→5095.700	43716.900→4851.700	1698731.000→719279.100

The selection versus full-sort bitwise oracle selection_matches_full_sort_bit_for_bit passed on the Windows PR-head build (1 passed, 0 failed). No cases in this table were unmeasured. Formula timings include argument construction, which dominates the 10,000-value cases.

[vangel76]

Thank you for the detailed measurements, they were very useful.

I reproduced your findings on Linux (one pinned core, release build, instruction counts as the main metric). The cause was sorted input: the standard sort recognises an ascending or descending run in one pass, while the selection still does several passes. So on sorted, reverse-sorted and tie-block input (values repeated in sorted blocks) the branch needed 1.2 to 1.8 times the instructions of main. This also covers the formula-level slowdown with array-lifted LARGE (k = 1, 2, 3, 10), since each k repeats that work. Your small-input and 1,000,000 results were confirmed as faster. I could not reproduce a slowdown for randomly ordered duplicate-heavy data, which is 0.5 to 0.7 times main here; the tie-block shape (sorted runs of equal values) did regress and is fixed now.

Change (head 8b330ac): before selecting, one cheap pass checks whether the data is already ascending or descending, and if so the value is read straight from it. The result is still bit-identical to a full sort, and the oracle test selection_matches_full_sort_bit_for_bit now also covers ascending, descending and nearly ascending data.

Instructions per call, branch relative to main (1.00 = same):

Input	Before fix	After fix
10,000 sorted, MEDIAN / PERCENTILE.INC / LARGE	1.63 / 1.74 / 1.26	1.03 / 1.03 / 1.03
10,000 reverse-sorted, same three	1.50 / 1.69 / 1.31	1.02 / 1.02 / 1.02
10,000 sorted blocks of 100 equal values, MEDIAN	1.62	1.03
10,000 sorted, range + LARGE/SMALL with 4 k values	1.21 / 1.42	1.03 / 1.03
10,000 reverse-sorted, range + LARGE/SMALL with 4 k values	1.25 / 1.14	1.03 / 1.03
10,000 random, MEDIAN core / range + LARGE 4 k	0.23 / 0.18	unchanged
128 random, MEDIAN core / range + LARGE 4 k	0.49 / 0.42	unchanged
1,000,000 random, MEDIAN core / range + LARGE 4 k	0.16 / 0.12	unchanged

One caveat: a "pipe organ" shape (values rising then falling) is still about 1.5 times main for MEDIAN in instruction count, although wall time at 10,000 values was equal or lower. I left that alone because it is rare and fixing it would need a more complex algorithm. The PR description has the updated table.

[FredApps]

Re-review at 8b330ac (previously commented at 890870c).

Since then: two merges of main and two commits (4523b81, 8b330ac) that read already-sorted input directly instead of selecting. I read the whole current diff of stat/descriptive.rs and stat/tests.rs.

Checked:

• sorted_if_monotone checks ascending, then descending, in total_cmp order, each stopping at the first out-of-order pair, and reverses a descending slice. Elements that compare equal under total_cmp are bit-identical (it separates -0.0 from 0.0 and orders NaNs by bits), so a reversed descending slice equals what the full sort produced.
• median: monotone input takes the old middle read; otherwise select_nth_unstable_by(n/2), and for an even count the lower neighbour is the maximum of the lower partition, which is the element at n/2 - 1 in sorted order (the partition is non-empty because n >= 2 there).
• kth: idx is always in range after the existing k checks (NaN, below 1 and above len still return #NUM!), and selection at that index returns the same element as sorted(v)[idx].
• interp: still #NUM! when the floor index is out of range; the next order statistic is the minimum of the upper partition (or none, giving lo), which is what s.get(i + 1) returned, and the interpolation arithmetic is unchanged, so the result is bit-identical. percentile_inc and percentile_exc compute pos before moving the vector and keep their range checks.
• selection_matches_full_sort_bit_for_bit compares median, kth and both percentiles against kept copies of the old implementations on random data with ties, signed zeros, infinities, NaNs and both signs, at lengths 0 to 4,096, including already ascending, already descending and nearly ascending inputs (the monotone shortcut and its fallback).

No findings. I did not run the tests or the timings. Approval still needs a run on the current head and target.