fix(statistical): MAX/MIN/MAXA/MINA compare dates instead of ignoring them - #783
Merged
Conversation
MAX and MIN folded only `Value::Number`, so a `Value::Date` element was
skipped entirely. A date-only array reached the end with no result:
`=MAX({DATE(2020,1,1),DATE(2021,1,1)})` was #REF! and `=MIN(...)` a
silent 0 that renders as a date in 1899. MAXA/MINA answered #N/A on the
same input — three different answers across four functions. The
everyday form of this is `=MAX(A1:A10)` over a column of dates.
Dates now fold in as their bare serial in every position (direct
argument, array element, nested/range-materialized array), so all four
functions agree: MAX/MAXA take the largest serial, MIN/MINA the
smallest, with no special casing — a small plain number beats every date
for MIN and MINA on a mixed range. The result is date-typed whenever a
date took part, including when a plain number won the comparison.
Captured in Google Sheets over a date-only column, a date/number column,
and array literals of both shapes; the typing was read back through the
cell holding the result. The fixture rows land separately.
Two things deliberately left alone, both unprobed and both unchanged by
this commit: an all-blank range (MAX #REF!, MIN 0, MAXA/MINA #N/A), and
a zone-aware value beside a date in MAXA/MINA, which do not consult the
zoned path MAX/MIN use.
The remaining `_ =>` catch-alls in the four folds are replaced by
explicit variant arms, so a `Value` variant added later is a compile
error at each fold rather than a silent skip.
closes #776
Review follow-up on the MAX/MIN/MAXA/MINA date change. The behaviour is
unchanged — this corrects what the comments claim about the evidence
behind it, and refreshes two summaries the change had made stale.
The earlier wording said the typing was captured "over a date-only
column, a date/number column, and array literals of both shapes". That
overstates the read-back. The capture types the result only through the
cell holding it for the two *range* forms; the array-literal and
direct-argument rows come back reporting `number` purely because the
harness reads them through an `INDEX(...,1,1)` wrapper that drops the
cell's date format. Values are captured for every form, typing for the
range forms alone, and the literal-form typing is extrapolated. All four
doc blocks now say so, and the two integration tests that assert
`ISDATE(MAX({DATE(...),DATE(...)}))` carry the same marker instead of
reading as probed.
Stale one-liners fixed: MAX/MIN's header said "Array elements: Numbers
only", and MAXA/MINA's in-array comment omitted dates, both contradicting
the paragraphs beneath them.
Two known consequences are now cross-referenced from all four doc blocks
rather than living only in a hand-off note:
- COUNT does not count dates, so `=COUNT(MAX(<date range>))` flips 1 to
0. Pre-existing in COUNT — its fold still uses a `_ => {}` catch-all,
which a probe variant added to `Value` confirms is not flagged on
either side of this change — and this change makes it reachable (#780).
- MAXA/MINA silently drop a Zoned beside a Date while MAX/MIN error on
the same input, because MAXA/MINA never consult `zoned_extreme`.
Unprobed in both directions (#781).
The npm surface change is stated where a consumer sees it: the
@truecalc/core README now notes that MAX/MIN/MAXA/MINA return
`{type:'date'}` where they returned `{type:'number'}`, with the same
serial-number `value` encoding.
refs #776
Contributor
Test Coverage by Category
✓ = 100% passing · ⚠ = known deviation · The ~79,449 total counts formula evaluations (each conformance row and each property case = 1). GitHub Checks reports 3,768 Rust test functions: 2,922 unit + 159 property functions (shown as cases above) + 687 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.
1 participant
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.
closes #776
Summary
All four functions folded only
Value::Number, so dates fell through entirely:=MIN(A1:A10)over a date column returning a silent0is the exact hazard #771 was filed about — a believable wrong answer rather than a visible error. It has been there the whole time.What Sheets actually does
Captured, all four functions agreeing:
No special casing — dates compare as bare serials, so a plain
5beats every date forMIN. But the result is date-typed on both the date-only and the mixed range, including where the number won and would render as 1900-01-04.The GAS harness wraps every formula as
INDEX((<formula>),1,1), which strips the automatic date format. A control asserting=DATE(2020,1,1)— which must be a date in Sheets — came back typednumber.So no direct probe row can answer "date or bare serial". It would have silently reported serial for everything, and this fix would have been built on it. The date cases are therefore evaluated into real cells (
Data!Q1,Data!Q2) and read back through a reference, which preserves the format.Consequence, stated in all four doc blocks: values are captured for every form; typing is captured only for the range forms. Literal-form typing is extrapolated and labelled as such, in the comments and in both integration tests.
crates/wasm/src/lib.rsmapsValue::DatetoEvalResult::Date, so=MAX(dateColumn)goes from{type:"number"}to{type:"date"}.valueencoding unchanged. Noted incrates/wasm/README.mdbeside the existing surface-change note, so a consumer sees it where they'd look.The rebase onto #775 found four test failures a clean merge had hidden
This branch conflicts with #775 by design. The fold bodies auto-merged; the conflicts were all in doc comments. But four tests then failed — and the test files auto-merged without conflicting at all.
#775 pinned an invariant using a
Datecompanion, with the note "if that ever moves it has to move deliberately, not as fallout from this rule." It moved. And there is no drop-in replacement: after this change, no variant reachesMAX's numberless#REF!through a populated array — text and booleans setarray_had_content, dates now contribute a number, sparklines set their own flag, and aZonedis intercepted before the loop. The assertion was retargeted to the date, renamed, and the reason recorded. The invariant is still pinned and still discriminating: the answer is the date, not the blank-only0.Fixes went in via
--fixup/--autosquash, so commit 1 is green standalone (2922 lib tests atHEAD~1) — no bisect hazard.Verified after the rebase, not assumed
The compile guard survived — all 8 sites. Re-added a probe variant to
Value; every function still fails to compile at both its top-level loop and its recursive fold:count/mod.rsis still not flagged — #780's catch-all remains live, as filed.#775's behaviour intact.
stat_helpers.rsbyte-identical tomain; all four still return0for blank-only arrays across three shapes.A 327-row differential captured before the rebase and re-run after: the diff is exactly 9 rows, all of them #775's blank-only corrections. Every date row, every typing probe, and every neighbour row (
COUNT,SUM,AVERAGE) byte-identical.Review
Valueargument tree, not by pattern-matching formula text. 0 date-free rows changed. 0 rows turned a non-error into an error.Two follow-ups this exposed
COUNTdoes not count dates, so=COUNT(MAX(<date range>))flips1→0. Pre-existing; this change makes it reachable.MAXA/MINAsilently drop aZonedbeside aDatewhileMAX/MINerror on the same input. Unprobed both ways.They come off the conformance-fixtures pipeline in a separate fixtures-only PR, for two forced reasons: they fail until this code exists, and CI rejects any PR touching both fixture TSVs and code. A reviewer working from this repo alone cannot verify the Sheets answers.
How to test
Directly — over a column of dates,
=MINreturns the earliest and=MAXthe latest, both date-typed. Before:0and#REF!.Test plan
cargo test --workspace— 3775 passed, 89 suitescargo clippy --workspace -- -D warnings— cleancargo nextest run --workspace --profile ci— conformance 10521/10527, same 6 pre-existing failuresvalue_completeness.rsRelated
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.