fix(statistical): MAX/MIN/MAXA/MINA compare dates instead of ignoring them

crates/core/src/eval/functions/statistical/max/mod.rs

@@ -1,8 +1,9 @@
 use crate::types::{ErrorKind, Value};
 
 /// `MAX(value1, ...)` — largest numeric value in the arguments.
-/// Direct args: Numbers, Bool (TRUE=1, FALSE=0), parseable text coerced to number.
-/// Array elements: Numbers only; text/Bool → skip; errors propagate.
+/// Direct args: Numbers, Dates, Bool (TRUE=1, FALSE=0), parseable text coerced
+/// to number.
+/// Array elements: Numbers and Dates; text/Bool → skip; errors propagate.
 ///
 /// "No numbers" is *two* rules, not one:
 ///
@@ -21,11 +22,47 @@ use crate::types::{ErrorKind, Value};
 /// `array_had_content` is deliberate: a blank sitting next to something else
 /// numberless changes nothing.
 ///
-/// This change moves the blank-only array and nothing else. `array_had_content`
-/// is still set by text and booleans alone, so it exempts exactly what those
-/// captures cover and makes no claim past them.
+/// `array_had_content` is still set by text and booleans alone, so it exempts
+/// exactly what those captures cover and makes no claim past them. A zoned
+/// instant is the remaining numberless case: unprobed, and still `#REF!`.
 ///
 /// [`stat_helpers::is_blank_only_array`]: super::stat_helpers::is_blank_only_array
+///
+/// **Dates participate as bare serials** and carry their type out. A date-only
+/// range answers the latest date, a date beside a plain number is compared on
+/// the serial with no special casing, and the result is date-typed whenever a
+/// date took part — even when a plain number won the comparison.
+///
+/// Captured and extrapolated are not the same thing here, so keep them apart:
+///
+/// - **Values** are captured in Google Sheets for every form — a date-only
+///   column, a date/number column, array literals of both shapes, and dates
+///   passed as direct arguments.
+/// - **Typing** is captured for the *range* forms only, read back through the
+///   cell that holds the result (`=MAX(<date-only range>)` and
+///   `=MAX(<date/number range>)` both come back `date`). The literal and
+///   direct-argument rows report `number`, but that is an artifact of the
+///   capture harness reading them through an `INDEX(...,1,1)` wrapper, which
+///   drops the cell's date format — it is not a Sheets answer. So the date
+///   typing of `=MAX({DATE(...),DATE(...)})` is **extrapolated** from the
+///   range forms, not probed.
+///
+/// **None of those rows are in this repo yet.** They come off the
+/// conformance-fixtures pipeline and land in a separate fixtures-only PR —
+/// they fail until this code exists, and CI rejects a PR that mixes fixture
+/// TSVs with code. Same arrangement as the blank-only rows described on
+/// `stat_helpers::is_blank_only_array`. A reviewer working from this repo
+/// alone can check the unit tests and this comment; the Sheets answers
+/// themselves have to be taken from that pipeline.
+///
+/// Two consequences are filed rather than fixed here:
+///
+/// - `COUNT` does not count dates, so `=COUNT(MAX(<date range>))` answers 0
+///   where it used to answer 1 — a pre-existing `COUNT` gap this change makes
+///   reachable. See #780.
+/// - `MAXA`/`MINA` silently drop a `Zoned` sitting beside a `Date`, where
+///   `MAX`/`MIN` route the same input through `zoned_extreme` and error.
+///   Unprobed on both sides. See #781.
 pub fn max_fn(args: &[Value]) -> Value {
     if args.is_empty() {
         return Value::Error(ErrorKind::NA);
@@ -39,12 +76,17 @@ pub fn max_fn(args: &[Value]) -> Value {
     let mut had_array = false;
     let mut array_had_content = false;
     let mut skipped_sparkline = false;
+    let mut saw_date = false;
     for arg in args {
         match arg {
             Value::Sparkline(_) => skipped_sparkline = true,
             Value::Number(n) => {
                 result = Some(result.map_or(*n, |cur: f64| cur.max(*n)));
             }
+            Value::Date(n) => {
+                saw_date = true;
+                result = Some(result.map_or(*n, |cur: f64| cur.max(*n)));
+            }
             Value::Bool(b) => {
                 let n = if *b { 1.0 } else { 0.0 };
                 result = Some(result.map_or(n, |cur: f64| cur.max(n)));
@@ -72,13 +114,18 @@ pub fn max_fn(args: &[Value]) -> Value {
                     &mut result,
                     &mut skipped_sparkline,
                     &mut array_had_content,
+                    &mut saw_date,
                 ) {
                     return e;
                 }
             }
             Value::Error(e) => return Value::Error(e.clone()),
             Value::ErrorMsg(e, m) => return Value::ErrorMsg(e.clone(), m.clone()),
-            _ => {}
+            // Listed rather than a catch-all so a new `Value` variant is a
+            // compile error here instead of a silent skip. A `Zoned` only
+            // reaches this loop when no other argument was zone-aware, which
+            // `zoned_extreme` above has already ruled on.
+            Value::Zoned(_) => {}
         }
     }
     // A skipped sparkline is not "nothing usable": the aggregate had something
@@ -91,7 +138,8 @@ pub fn max_fn(args: &[Value]) -> Value {
     // An array holding text or booleans answers 0 (`=MAX({"a","b"})` and
     // `=MAX({TRUE,FALSE})` are both 0). `array_had_content` is set by exactly
     // those two variants and nothing else, so it exempts what that capture
-    // covers and makes no claim beyond it.
+    // covers and makes no claim beyond it. Dates never reach here: they
+    // contribute a number, so `result` is `Some` whenever one was seen.
     if had_array && !array_had_content && result.is_none() {
         // An array of nothing but blanks is a further exemption, and it is
         // captured: `=MAX(A1:A3)` over empty cells is 0, the same answer MIN,
@@ -104,7 +152,13 @@ pub fn max_fn(args: &[Value]) -> Value {
         }
         return Value::Error(ErrorKind::Ref);
     }
-    Value::Number(result.unwrap_or(0.0))
+    match result {
+        // A date anywhere in scope makes the answer date-typed, whether or not
+        // the date is the value that won.
+        Some(n) if saw_date => Value::Date(n),
+        Some(n) => Value::Number(n),
+        None => Value::Number(0.0),
+    }
 }
 
 /// Recursively fold a nested array's numbers into `result` for MAX's
@@ -115,31 +169,46 @@ pub fn max_fn(args: &[Value]) -> Value {
 /// and `=MAX(Data!K1:K1)` are both 0). The flag is what distinguishes "skipped a
 /// sparkline" from "saw nothing usable at all", which stay different answers.
 ///
+/// A `Date` folds in as its bare serial and raises `saw_date`, which types the
+/// answer; it never touches `had_content`, since a date always leaves a number
+/// behind and so can never reach the numberless rule.
+///
 /// `had_content` is set by text and booleans *only* — the two variants the
 /// capture covers (`=MAX({"a","b"})` and `=MAX({TRUE,FALSE})` are both 0).
-/// It is deliberately not a catch-all: every other non-numeric variant, most
-/// notably `Date`, leaves it alone and so keeps the `#REF!` MAX has always
-/// answered. Listing the variants rather than falling through also stops a
-/// future `Value` kind inheriting content-hood by accident. An all-blank array
-/// no longer ends at `#REF!` either, but it gets there without this flag — see
-/// the blank-only check in `max_fn`.
+/// It is deliberately not a catch-all: every other non-numeric variant leaves
+/// it alone and so keeps the `#REF!` MAX has always answered. Listing the
+/// variants rather than falling through also stops a future `Value` kind
+/// inheriting content-hood by accident. Two variants no longer end at `#REF!`,
+/// and neither gets there through this flag: an all-blank array is decided by
+/// the blank-only check in `max_fn`, and a `Date` contributes a number so the
+/// numberless rule is never reached at all.
 fn max_array_into(
     elems: &[Value],
     result: &mut Option<f64>,
     skipped_sparkline: &mut bool,
     had_content: &mut bool,
+    saw_date: &mut bool,
 ) -> Result<(), Value> {
     for elem in elems {
         match elem {
             Value::Number(n) => {
                 *result = Some(result.map_or(*n, |cur: f64| cur.max(*n)));
             }
+            Value::Date(n) => {
+                *saw_date = true;
+                *result = Some(result.map_or(*n, |cur: f64| cur.max(*n)));
+            }
             Value::Text(_) | Value::Bool(_) => *had_content = true,
             Value::Sparkline(_) => *skipped_sparkline = true,
             Value::Error(e) => return Err(Value::Error(e.clone())),
             Value::ErrorMsg(e, m) => return Err(Value::ErrorMsg(e.clone(), m.clone())),
-            Value::Array(inner) => max_array_into(inner, result, skipped_sparkline, had_content)?,
-            _ => {}
+            Value::Array(inner) => {
+                max_array_into(inner, result, skipped_sparkline, had_content, saw_date)?
+            }
+            // Listed rather than a catch-all so a new `Value` variant is a
+            // compile error here instead of inheriting "skipped, and not
+            // content either" by accident.
+            Value::Empty | Value::Zoned(_) => {}
         }
     }
     Ok(())

crates/core/src/eval/functions/statistical/max/tests/edge.rs

@@ -66,15 +66,25 @@ fn max_array_of_only_blanks_is_zero() {
 }
 
 #[test]
-fn max_blank_beside_something_else_numberless_is_still_ref_error() {
+fn max_blank_beside_something_else_does_not_become_the_blank_only_zero() {
     // The blank-only rule is decided over the arguments as a whole, not by a
     // per-element flag, so a blank cannot on its own pull an array that holds
-    // something else into the blank-only 0. Pinned with a date element because
-    // that is where MAX leaves such an array today; if that ever moves it has
-    // to move deliberately, not as fallout from this rule.
+    // something else into the blank-only 0.
+    //
+    // This was pinned with a date element and `#REF!`, on the note that "if
+    // that ever moves it has to move deliberately, not as fallout from this
+    // rule". It moved deliberately: dates now participate, so this array
+    // answers the date. The invariant the pin exists for is unchanged and
+    // still discriminating — the answer is the date, *not* the blank-only 0.
+    //
+    // There is no longer any variant that reaches MAX's numberless `#REF!`
+    // through a populated array: text and booleans set `array_had_content`,
+    // dates contribute a number, a sparkline sets its own flag, and a zoned
+    // instant is intercepted by `zoned_extreme` before the loop runs. The
+    // empty-array assertion below is what still carries the `#REF!` side.
     assert_eq!(
         max_fn(&[Value::Array(vec![Value::Empty, Value::Date(43831.0)])]),
-        Value::Error(ErrorKind::Ref)
+        Value::Date(43831.0)
     );
     // An empty array argument stays #REF! even alongside an all-blank one.
     assert_eq!(
@@ -84,24 +94,65 @@ fn max_blank_beside_something_else_numberless_is_still_ref_error() {
 }
 
 #[test]
-fn max_array_of_only_dates_is_unchanged_at_ref_error() {
-    // `max_array_into` folds only `Value::Number`, so a date-only array has
-    // never produced a result and has always answered #REF!. That is almost
-    // certainly wrong against Sheets — but it is pre-existing, unprobed, and
-    // must not be quietly turned into a plausible-looking 0 by the
-    // text-and-boolean carve-out. `had_content` is set by text and booleans
-    // only, never by a catch-all, and this pins that.
+fn max_array_of_only_dates_returns_the_latest_date() {
+    // Was #REF! until dates were captured: a date-only array now answers the
+    // largest serial, date-typed. Replaces the pin that recorded the old
+    // #REF! as unprobed.
     assert_eq!(
         max_fn(&[Value::Array(vec![
             Value::Date(43831.0),
             Value::Date(44197.0)
         ])]),
-        Value::Error(ErrorKind::Ref)
+        Value::Date(44197.0)
     );
     // Same for a date arriving through a nested-row range materialization.
     assert_eq!(
         max_fn(&[Value::Array(vec![Value::Array(vec![Value::Date(43831.0)])])]),
-        Value::Error(ErrorKind::Ref)
+        Value::Date(43831.0)
+    );
+}
+
+#[test]
+fn max_dates_compare_as_bare_serials_and_type_the_answer() {
+    // A plain number and a date are compared on the serial with no special
+    // casing, and the answer is date-typed because a date took part — even
+    // when the plain number is the one that won (that is the MIN direction;
+    // pinned in min's tests, mirrored here for the losing-date direction).
+    assert_eq!(
+        max_fn(&[Value::Array(vec![Value::Date(43831.0), Value::Number(5.0)])]),
+        Value::Date(43831.0)
+    );
+    // Direct (non-array) arguments take the same rule.
+    assert_eq!(
+        max_fn(&[Value::Date(43831.0), Value::Date(44197.0)]),
+        Value::Date(44197.0)
+    );
+    assert_eq!(
+        max_fn(&[Value::Date(43831.0), Value::Number(5.0)]),
+        Value::Date(43831.0)
+    );
+    // No date in scope: the answer stays a plain number.
+    assert_eq!(
+        max_fn(&[Value::Number(5.0), Value::Number(1.0)]),
+        Value::Number(5.0)
+    );
+}
+
+#[test]
+fn max_date_beside_a_blank_no_longer_falls_into_the_ref_rule() {
+    // A date leaves a number behind, so the "populated but numberless" #REF!
+    // rule can no longer be reached with a date in scope.
+    assert_eq!(
+        max_fn(&[Value::Array(vec![Value::Date(43831.0), Value::Empty])]),
+        Value::Date(43831.0)
+    );
+    // An all-blank array is a separate, captured case and answers 0, not the
+    // date rule and not #REF! — see `max_array_of_only_blanks_is_zero`. Kept
+    // here as the contrast: a date in scope types the answer, an array with
+    // nothing in it but blanks does not.
+    assert_eq!(
+        max_fn(&[Value::Array(vec![Value::Empty, Value::Empty])]),
+        Value::Number(0.0)
     );
 }