fix: preserve column orientation for vertical ranges - #763
Merged
Conversation
resolve_range materialized every range as a fully flat, row-major Value::Array, so a vertical (single-column, multi-row) range lost its orientation before core's evaluator ever saw it. An elementwise op like =A1:A3*2 then spilled as a 1xN row instead of the Nx1 column Google Sheets produces. resolve_range now materializes a vertical range as core's standard Nx1 nested-array shape (a row per element, one column each) instead of a flat array — but only for that shape; single-row and 2-D ranges are unchanged. Nesting a range this way exposed that AVERAGE, MAX, MIN, and the SUMIF/COUNTIF-family criterion flattener only unwrapped one level of array nesting, so a vertical range's aggregation broke (AVERAGE -> #DIV/0!, MAX -> #REF!, SUMIF -> 0). Made those four fully recursive so they walk arbitrarily nested arrays, matching how SUM/COUNT/COUNTA and the financial-function flatteners already behave. closes #724
The vertical-range nesting fix (previous commit) only patched AVERAGE, MAX, MIN, and criterion::flatten_to_vec — the functions caught by pinned fixtures. Every other statistical function that flattens its array argument by hand (LARGE, SMALL, MODE, MODE.MULT, the PERCENTILE/ QUARTILE/PERCENTRANK family via their shared helper, RANK/RANK.AVG/ RANK.EQ, GEOMEAN, HARMEAN, TRIMMEAN, MAXA, MINA) only unwrapped one level of Value::Array, so a vertical range's new Nx1 nested shape silently vanished into "no recognized values" and these returned #NUM!/#N/A instead of the real answer. Make each of those local flatten helpers recurse into nested arrays, matching the recursive pattern already used by AVERAGE/MAX/MIN. Behavior for non-nested inputs (scalars, flat arrays, array literals) is unchanged. Extends the vertical_range_spill_tests.rs regression coverage to pin SMALL/LARGE/GEOMEAN/RANK/MAXA/MINA/MODE/PERCENTRANK over a vertical range.
Contributor
Test Coverage by Category
✓ = 100% passing · ⚠ = known deviation · The ~79,362 total counts formula evaluations (each conformance row and each property case = 1). GitHub Checks reports 3,606 Rust test functions: 2,835 unit + 159 property functions (shown as cases above) + 612 conformance/integration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Elementwise operations over a vertical range (e.g.
=A1:A3*2where A1:A3 is a 1-column, 3-row range) now spill down (an Nx1 column) instead of sideways (a 1xN row), matching Google Sheets.resolve_rangenow materializes a single-column, multi-row range as a nested Nx1 array (one extraArraylevel per cell) instead of a flat row. Making that shape change safe required updating every function that flattens aValue::Arrayargument by hand to fully recurse into nested arrays, not just unwrap one level:flatten_to_vec) — the 4 functions caught by pinned conformance fixtures.#NUM!/#N/Auntil fixed here.No existing conformance fixtures were touched.
How to test
From the
corerepo root:Or manually, in a workbook: set
A1:A3to1, 2, 3, then inB1enter=A1:A3*2— it should spill down intoB1:B3as2, 4, 6(not sideways intoB1:D1). Then try=SMALL(A1:A3,1),=GEOMEAN(A1:A3),=RANK(A1,A1:A3),=MAXA(A1:A3)over the same vertical range — each should return its real value instead of#NUM!/#N/A.Review
Independent review: 1 finding fixed. The review flagged that the initial fix only patched the 4 functions caught by pinned fixtures (AVERAGE, MAX, MIN, criterion::flatten_to_vec) and left ~17 other statistical functions with a one-level-only array unwrap, which the reviewer confirmed regressed (
SMALL,GEOMEAN,RANK,MAXAall returned#NUM!/#N/Aover a plain vertical range on the pre-fix branch). All of those functions' local flatten helpers now recurse into nested arrays, and regression tests were added pinning the previously-broken cases.Test plan
cargo build --workspacecargo clippy --workspace -- -D warnings— cleancargo test --workspace— 3613 passedcargo test -p truecalc-workbook --test vertical_range_spill_tests— 2 passed (new regression coverage for SMALL/LARGE/GEOMEAN/RANK/MAXA/MINA/MODE/PERCENTRANK over a vertical range)closes #724
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.