Address Rich-T-kid review on #23128: assert! + comment tightening

* Dispatcher (`mod.rs`): change the `list_size < 0` guard from a
  `return not_impl_err!` to a bare `assert!`. `group_column_supported_type`
  already rejects negative sizes at the schema-check layer, so any
  reachable path with `list_size < 0` is a programmer bug rather than
  an unsupported case worth returning gracefully. Split the existing
  test into an allow-list case (checked via `bool`) and a dispatcher
  case (verified with `#[should_panic]`) so both surfaces stay covered.

* Constructor (`fixed_size_list.rs`): fold the multi-line invariant
  comment above `assert!` into the assert message itself, which now
  reads "requires non-negative list size (allow-list / dispatcher
  should have rejected earlier)". Simpler to read at the panic site,
  and the rationale surfaces exactly where a future reader would want
  it (the panic backtrace). Same trim on `list_len_usize`.

* Dispatcher consistency-fuzz corpus (`mod.rs`): shorten the
  FixedSizeList<non-primitive> comment to a one-liner pointing at
  EPIC #22715, instead of the two-line prose explanation.

Tests: `cargo test -p datafusion-physical-plan --lib multi_group_by`
now runs 42/42 (up from 41), including the split negative-size cases
and the new `#[should_panic]` guarantee. Clippy clean.

Author: zhuqi-lucas

datafusion/physical-plan/src/aggregates/group_values/multi_group_by/fixed_size_list.rs

@@ -60,14 +60,9 @@ where
                 "FixedSizeListGroupValueBuilder built with non-FixedSizeList type {other:?}"
             ),
         };
-        // `group_column_supported_type` and `make_group_column` both reject
-        // negative `list_len` upstream so this branch should be unreachable
-        // for any schema that survives the allow-list. Assert defensively
-        // to fail fast (and with a clear message) if a direct caller ever
-        // bypasses the factory with an invalid Arrow type.
         assert!(
             list_len >= 0,
-            "FixedSizeListGroupValueBuilder requires non-negative list size, got {list_len}"
+            "FixedSizeListGroupValueBuilder requires non-negative list size (allow-list / dispatcher should have rejected earlier), got {list_len}"
         );
         let child = PrimitiveGroupValueBuilder::<T, true>::new(field.data_type().clone());
         Self {
@@ -79,12 +74,9 @@ where
         }
     }
 
-    /// Lossless widening to `usize`. The constructor asserts
-    /// `list_len >= 0`, so the conversion is well-defined. We still go
-    /// through `try_from` rather than `as usize` so any future
-    /// invariant break surfaces as a panic with a clear message rather
-    /// than a silent wrap.
     fn list_len_usize(&self) -> usize {
+        // `try_from` (vs `as usize`) so any future invariant break
+        // surfaces as a panic instead of a silent wrap.
         usize::try_from(self.list_len)
             .expect("list_len validated >= 0 in `new`; conversion to usize cannot fail")
     }
@@ -439,14 +431,11 @@ mod tests {
 
     /// `FixedSizeList` carries its size as `i32`; the enum lets callers
     /// construct a negative size programmatically even though Arrow
-    /// considers it invalid. Both the supported-type allow-list and the
-    /// dispatcher must reject such a type so the cast inside the
-    /// builder cannot wrap to a huge `usize` and trigger panics / OOM.
+    /// considers it invalid. The supported-type allow-list must reject
+    /// such a type so it never reaches the builder path.
     #[test]
-    fn negative_list_size_is_rejected_by_allow_list_and_dispatcher() {
-        use crate::aggregates::group_values::multi_group_by::{
-            group_column_supported_type, make_group_column,
-        };
+    fn negative_list_size_rejected_by_allow_list() {
+        use crate::aggregates::group_values::multi_group_by::group_column_supported_type;
         use arrow::datatypes::Field;
 
         let invalid = DataType::FixedSizeList(
@@ -458,18 +447,23 @@ mod tests {
             !group_column_supported_type(&invalid),
             "allow-list must reject FixedSizeList(_, -1)"
         );
+    }
 
-        let field = Field::new("col", invalid, true);
-        let result = make_group_column(&field);
-        let err = match result {
-            Ok(_) => panic!("dispatcher must reject FixedSizeList with negative size"),
-            Err(e) => e,
-        };
-        let msg = err.to_string();
-        assert!(
-            msg.contains("negative size") && msg.contains("-1"),
-            "error should mention the invalid negative size, got: {msg}"
+    /// The dispatcher is a belt-and-suspenders guard for direct callers
+    /// that bypass the allow-list. A negative `list_size` must panic
+    /// there rather than wrap in the builder.
+    #[test]
+    #[should_panic(expected = "FixedSizeList requires non-negative size")]
+    fn negative_list_size_panics_in_dispatcher() {
+        use crate::aggregates::group_values::multi_group_by::make_group_column;
+        use arrow::datatypes::Field;
+
+        let invalid = DataType::FixedSizeList(
+            Arc::new(Field::new("item", DataType::Int32, true)),
+            -1,
         );
+        let field = Field::new("col", invalid, true);
+        let _ = make_group_column(&field);
     }
 
     #[test]

datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs

@@ -1096,16 +1096,15 @@ fn make_group_column(field: &Field) -> Result<Box<dyn GroupColumn>> {
             }
         }
         DataType::FixedSizeList(ref child_field, list_size) => {
-            // Defense in depth against an invalid Arrow type that the
-            // allow-list might have missed: negative `list_size` would
-            // wrap when cast to `usize` and trigger panics / OOM in the
-            // builder. Reject explicitly here as well so direct callers
-            // of `make_group_column` fail safely.
-            if list_size < 0 {
-                return not_impl_err!(
-                    "FixedSizeList with negative size {list_size} not supported in GroupValuesColumn"
-                );
-            }
+            // `group_column_supported_type` already rejects negative
+            // `list_size`; this assert is a defensive belt-and-suspenders
+            // check so a direct caller of `make_group_column` that bypasses
+            // the allow-list can't wrap the `i32 as usize` cast in the
+            // builder into a huge value.
+            assert!(
+                list_size >= 0,
+                "FixedSizeList requires non-negative size, got {list_size}"
+            );
             // `group_column_supported_type` already restricts the child to
             // the primitive subset supported here. Any unsupported child
             // type returned `false` upstream and was routed to the
@@ -1430,9 +1429,8 @@ mod tests {
             DataType::Time64(arrow::datatypes::TimeUnit::Millisecond),
             DataType::Time32(arrow::datatypes::TimeUnit::Microsecond),
             DataType::Time32(arrow::datatypes::TimeUnit::Nanosecond),
-            // FixedSizeList<non-primitive>: only primitive children are
-            // covered by this PR; nested children depend on the
-            // List / Struct builders that follow in the EPIC sequence.
+            // FixedSizeList<non-primitive>: primitive-only in this PR;
+            // nested children land later in EPIC #22715.
             DataType::FixedSizeList(
                 Arc::new(Field::new("item", DataType::Utf8, true)),
                 2,