Address Copilot review on #23128: reject negative FixedSizeList size, drop per-compare allocation in equal_to

Four Copilot inline comments handled together:

* `group_column_supported_type` and `make_group_column` both now reject
  `FixedSizeList` with a negative `list_size`. Arrow defines that size
  as non-negative, but the enum lets callers construct a negative value
  programmatically; without this guard the `i32 as usize` cast inside
  the builder would wrap and risk panics / OOM. Guards are duplicated
  on both sides so direct callers of `make_group_column` (and the
  `supported_schema` planner gate) both fail safely. Consistency fuzz
  already covers the `make_group_column` side via the existing
  unsupported_cases path; an explicit dedicated test below covers both
  surfaces.

* `FixedSizeListGroupValueBuilder::new` asserts `list_len >= 0` so any
  direct construction that bypasses the factory fails fast with a
  clear message instead of silently producing a broken builder.
  `list_len_usize` now uses `usize::try_from(...).expect(...)` instead
  of `as usize`; with the constructor's assertion the conversion is
  provably infallible, but going through `try_from` means a future
  invariant break shows up as an explicit panic rather than a silent
  wrap.

* `equal_to` no longer calls `list_array.value(rhs_row)`, which
  constructs a sliced child `ArrayRef` on every comparison. The hot
  path now borrows `list_array.values()` and uses `value_offset(rhs_row)`
  to compute the child base index, mirroring the existing approach in
  `append_val`. Zero allocations per equality check.

New test: `negative_list_size_is_rejected_by_allow_list_and_dispatcher`
locks in the negative-size guard at both surfaces. 13/13 FSL tests
pass, 128/128 aggregates pass, clippy clean.

Author: zhuqi-lucas

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

@@ -60,6 +60,15 @@ 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}"
+        );
         let child = PrimitiveGroupValueBuilder::<T, true>::new(field.data_type().clone());
         Self {
             field,
@@ -70,8 +79,14 @@ 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 {
-        self.list_len as usize
+        usize::try_from(self.list_len)
+            .expect("list_len validated >= 0 in `new`; conversion to usize cannot fail")
     }
 }
 
@@ -91,11 +106,16 @@ where
             .as_any()
             .downcast_ref::<FixedSizeListArray>()
             .expect("FixedSizeListGroupValueBuilder called with non-FixedSizeList array");
-        let rhs_sublist: ArrayRef = list_array.value(rhs_row);
+        // Use the borrowed child array + `value_offset` (same approach as
+        // `append_val` below) instead of `list_array.value(rhs_row)`,
+        // which would allocate a fresh sliced `ArrayRef` on every
+        // equality check inside grouping's hot path.
+        let child_array = list_array.values();
         let list_len = self.list_len_usize();
         let lhs_base = lhs_row * list_len;
+        let rhs_base = list_array.value_offset(rhs_row) as usize;
         for j in 0..list_len {
-            if !self.child.equal_to(lhs_base + j, &rhs_sublist, j) {
+            if !self.child.equal_to(lhs_base + j, child_array, rhs_base + j) {
                 return false;
             }
         }
@@ -417,6 +437,41 @@ mod tests {
         assert_eq!(values.values(), &[1, 2, 3, 4, 5, 6]);
     }
 
+    /// `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.
+    #[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,
+        };
+        use arrow::datatypes::Field;
+
+        let invalid = DataType::FixedSizeList(
+            Arc::new(Field::new("item", DataType::Int32, true)),
+            -1,
+        );
+
+        assert!(
+            !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}"
+        );
+    }
+
     #[test]
     fn take_n_returns_prefix_and_shifts_remainder() {
         let input = fsl_array(&[

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

@@ -926,7 +926,15 @@ macro_rules! instantiate_primitive {
 fn group_column_supported_type(data_type: &DataType) -> bool {
     // FixedSizeList<primitive> is the only nested type currently supported.
     // List / LargeList / Struct will follow in subsequent PRs of #22715.
-    if let DataType::FixedSizeList(child_field, _) = data_type {
+    if let DataType::FixedSizeList(child_field, list_size) = data_type {
+        // Arrow defines `FixedSizeList` size as a non-negative `i32`, but
+        // the enum still lets callers construct a negative size. Reject
+        // here so the schema is routed to the `GroupValuesRows` fallback
+        // instead of risking a `negative_size as usize` wrap inside the
+        // builder. Keep this guard in lockstep with `make_group_column`.
+        if *list_size < 0 {
+            return false;
+        }
         return matches!(
             child_field.data_type(),
             DataType::Int8
@@ -1087,7 +1095,17 @@ fn make_group_column(field: &Field) -> Result<Box<dyn GroupColumn>> {
                 v.push(Box::new(BooleanGroupValueBuilder::<false>::new()));
             }
         }
-        DataType::FixedSizeList(ref child_field, _) => {
+        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 restricts the child to
             // the primitive subset supported here. Any unsupported child
             // type returned `false` upstream and was routed to the