Copilot commented on code in PR #379: URL: https://github.com/apache/arrow-dotnet/pull/379#discussion_r3725515333
########## test/Apache.Arrow.Compute.Tests/AggregationsTests.cs: ########## @@ -0,0 +1,144 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Linq; +using Apache.Arrow; +using Apache.Arrow.Compute; +using Xunit; + +namespace Apache.Arrow.Compute.Tests +{ + public class AggregationsTests + { + private static DoubleArray Doubles(params double?[] values) + { + var builder = new DoubleArray.Builder(); + foreach (double? v in values) + { + if (v.HasValue) builder.Append(v.Value); + else builder.AppendNull(); + } + return builder.Build(); + } + + private static Int32Array Ints(params int?[] values) + { + var builder = new Int32Array.Builder(); + foreach (int? v in values) + { + if (v.HasValue) builder.Append(v.Value); + else builder.AppendNull(); + } + return builder.Build(); + } + + [Fact] + public void Sum_Int32_NoNulls() + { + Assert.Equal(10, Ints(1, 2, 3, 4).Sum()); + } + + [Fact] + public void Sum_Int32_WithNulls() + { + Assert.Equal(4, Ints(1, null, 3).Sum()); + } + + [Fact] + public void Sum_Double_NoNulls() + { + Assert.Equal(6.5, Doubles(1.0, 2.0, 3.5).Sum()!.Value, 6); + } + + [Fact] + public void Min_Max_Double_NoNulls() + { + var a = Doubles(3.0, -1.0, 7.5, 2.0); + Assert.Equal(-1.0, a.Min()!.Value, 6); + Assert.Equal(7.5, a.Max()!.Value, 6); + } + + [Fact] + public void Min_Max_WithNulls_IgnoresNulls() + { + var a = Doubles(null, 5.0, null, 2.0, 9.0); + Assert.Equal(2.0, a.Min()!.Value, 6); + Assert.Equal(9.0, a.Max()!.Value, 6); + } + + [Fact] + public void Mean_Double_WithNulls_DividesByNonNullCount() + { + // (2 + 4) / 2 = 3, the null is excluded from both sum and count. + Assert.Equal(3.0, Doubles(2.0, null, 4.0).Mean()!.Value, 6); + } + + [Fact] + public void Mean_Int32_ReturnsDouble() + { + Assert.Equal(2.5, Ints(1, 2, 3, 4).Mean()!.Value, 6); + } + + [Fact] + public void SingleElement() + { + Assert.Equal(42.0, Doubles(42.0).Sum()!.Value, 6); + Assert.Equal(42.0, Doubles(42.0).Min()!.Value, 6); + Assert.Equal(42.0, Doubles(42.0).Max()!.Value, 6); + Assert.Equal(42.0, Doubles(42.0).Mean()!.Value, 6); + } + + [Fact] + public void Empty_AllReturnNull() + { + var empty = Doubles(); + Assert.Null(empty.Sum()); + Assert.Null(empty.Min()); + Assert.Null(empty.Max()); + Assert.Null(empty.Mean()); + } + + [Fact] + public void AllNull_AllReturnNull() + { + var allNull = Doubles(null, null, null); + Assert.Null(allNull.Sum()); + Assert.Null(allNull.Min()); + Assert.Null(allNull.Max()); + Assert.Null(allNull.Mean()); + } + + [Fact] + public void Large_FastPath_MatchesScalar() + { + const int n = 1_000_000; + var rng = new Random(17); + double[] data = Enumerable.Range(0, n).Select(_ => rng.NextDouble() * 100.0).ToArray(); + + var builder = new DoubleArray.Builder(); + builder.Append(data.AsSpan()); + DoubleArray array = builder.Build(); + + double scalar = 0.0; + for (int i = 0; i < n; i++) scalar += data[i]; + + // Fast (TensorPrimitives) path; allow small floating-point reorder tolerance. + Assert.Equal(scalar, array.Sum()!.Value, 3); + Assert.Equal(data.Min(), array.Min()!.Value, 9); Review Comment: `Assert.Equal(..., precision: 3)` on a 1,000,000-element sum can be flaky across hardware/JIT/SIMD implementations: different summation orders can differ by more than 1e-3 for large magnitudes even when both results are “correct” within expected floating-point error. Prefer an explicit absolute/relative tolerance check for the sum comparison. ########## src/Apache.Arrow.Compute/Aggregations.cs: ########## @@ -0,0 +1,406 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +#if NET8_0_OR_GREATER +using System.Numerics; +using System.Numerics.Tensors; +#endif + +namespace Apache.Arrow.Compute +{ + /// <summary> + /// Aggregation kernels over <see cref="PrimitiveArray{T}"/> (Sum/Min/Max/Mean). + /// </summary> + /// <remarks> + /// <para> + /// Null entries are skipped and do not contribute to the result. <c>Sum</c>, <c>Min</c>, + /// <c>Max</c> and <c>Mean</c> return <c>null</c> (<see cref="System.Nullable{T}"/>) when the + /// array is empty or contains no non-null elements. + /// </para> + /// <para> + /// On net8.0 and later the kernels are generic over <c>INumber<T></c> and, when the + /// array has no nulls, dispatch to <c>TensorPrimitives</c> for a SIMD-accelerated single + /// pass over the contiguous values buffer; when nulls are present they fall back to a correct, + /// validity-aware scalar loop. On netstandard2.0 and net462 (where generic math and + /// <c>TensorPrimitives</c> are unavailable) the kernels are provided as per-type overloads + /// (<see cref="Int32Array"/>, <see cref="Int64Array"/>, <see cref="FloatArray"/>, + /// <see cref="DoubleArray"/>) backed by scalar loops with the same null semantics. + /// </para> + /// </remarks> + public static class Aggregations + { +#if NET8_0_OR_GREATER + /// <summary>Sums the non-null elements. Returns null for an empty or all-null array.</summary> + public static T? Sum<T>(this PrimitiveArray<T> array) + where T : unmanaged, INumber<T> + { + if (array is null) throw new ArgumentNullException(nameof(array)); + + ReadOnlySpan<T> values = array.Values; + + if (values.Length == 0 || array.Length - array.NullCount == 0) + { + return null; + } + + if (array.NullCount == 0) + { + return TensorPrimitives.Sum(values); + } + + T acc = T.Zero; + for (int i = 0; i < values.Length; i++) + { + if (array.IsValid(i)) + { + acc += values[i]; + } + } + return acc; + } + + /// <summary>Returns the smallest non-null element, or null if there are no non-null elements.</summary> + public static T? Min<T>(this PrimitiveArray<T> array) + where T : unmanaged, INumber<T>, IMinMaxValue<T> + { + if (array is null) throw new ArgumentNullException(nameof(array)); + + ReadOnlySpan<T> values = array.Values; + + if (values.Length == 0 || array.Length - array.NullCount == 0) + { + return null; + } + + if (array.NullCount == 0) + { + return TensorPrimitives.Min(values); + } + + T min = T.MaxValue; + for (int i = 0; i < values.Length; i++) + { + if (!array.IsValid(i)) continue; + if (values[i] < min) { min = values[i]; } + } + return min; + } + + /// <summary>Returns the largest non-null element, or null if there are no non-null elements.</summary> + public static T? Max<T>(this PrimitiveArray<T> array) + where T : unmanaged, INumber<T>, IMinMaxValue<T> + { + if (array is null) throw new ArgumentNullException(nameof(array)); + + ReadOnlySpan<T> values = array.Values; + + if (values.Length == 0 || array.Length - array.NullCount == 0) + { + return null; + } + + if (array.NullCount == 0) + { + return TensorPrimitives.Max(values); + } + + T max = T.MinValue; + for (int i = 0; i < values.Length; i++) + { + if (!array.IsValid(i)) continue; + if (values[i] > max) { max = values[i]; } + } + return max; Review Comment: In the null-aware path, initializing `max` to `T.MinValue` can produce incorrect results for floating-point arrays containing NaN(s) (e.g., if the only non-null value is NaN, this returns `T.MinValue`). It also makes net8.0+ semantics diverge from the netstandard/net462 implementation below, which seeds from the first valid element. Seed `max` from the first valid element instead of using a sentinel. ########## test/Apache.Arrow.Compute.Tests/AggregationsTests.cs: ########## @@ -0,0 +1,144 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Linq; +using Apache.Arrow; +using Apache.Arrow.Compute; +using Xunit; + +namespace Apache.Arrow.Compute.Tests +{ + public class AggregationsTests + { + private static DoubleArray Doubles(params double?[] values) + { + var builder = new DoubleArray.Builder(); + foreach (double? v in values) + { + if (v.HasValue) builder.Append(v.Value); + else builder.AppendNull(); + } + return builder.Build(); + } + + private static Int32Array Ints(params int?[] values) + { + var builder = new Int32Array.Builder(); + foreach (int? v in values) + { + if (v.HasValue) builder.Append(v.Value); + else builder.AppendNull(); + } + return builder.Build(); + } + + [Fact] + public void Sum_Int32_NoNulls() + { + Assert.Equal(10, Ints(1, 2, 3, 4).Sum()); + } Review Comment: The PR description says the xUnit tests cover int32, int64, float, and double, but this file currently only exercises Int32Array and DoubleArray. Please add coverage for Int64Array and FloatArray (both null-free and with nulls) so the per-type scalar fallbacks (netstandard2.0/net462) and the generic net8.0+ path are both exercised for those types. ########## src/Apache.Arrow.Compute/Aggregations.cs: ########## @@ -0,0 +1,406 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +#if NET8_0_OR_GREATER +using System.Numerics; +using System.Numerics.Tensors; +#endif + +namespace Apache.Arrow.Compute +{ + /// <summary> + /// Aggregation kernels over <see cref="PrimitiveArray{T}"/> (Sum/Min/Max/Mean). + /// </summary> + /// <remarks> + /// <para> + /// Null entries are skipped and do not contribute to the result. <c>Sum</c>, <c>Min</c>, + /// <c>Max</c> and <c>Mean</c> return <c>null</c> (<see cref="System.Nullable{T}"/>) when the + /// array is empty or contains no non-null elements. + /// </para> + /// <para> + /// On net8.0 and later the kernels are generic over <c>INumber<T></c> and, when the + /// array has no nulls, dispatch to <c>TensorPrimitives</c> for a SIMD-accelerated single + /// pass over the contiguous values buffer; when nulls are present they fall back to a correct, + /// validity-aware scalar loop. On netstandard2.0 and net462 (where generic math and + /// <c>TensorPrimitives</c> are unavailable) the kernels are provided as per-type overloads + /// (<see cref="Int32Array"/>, <see cref="Int64Array"/>, <see cref="FloatArray"/>, + /// <see cref="DoubleArray"/>) backed by scalar loops with the same null semantics. + /// </para> + /// </remarks> + public static class Aggregations + { +#if NET8_0_OR_GREATER + /// <summary>Sums the non-null elements. Returns null for an empty or all-null array.</summary> + public static T? Sum<T>(this PrimitiveArray<T> array) + where T : unmanaged, INumber<T> + { + if (array is null) throw new ArgumentNullException(nameof(array)); + + ReadOnlySpan<T> values = array.Values; + + if (values.Length == 0 || array.Length - array.NullCount == 0) + { + return null; + } + + if (array.NullCount == 0) + { + return TensorPrimitives.Sum(values); + } + + T acc = T.Zero; + for (int i = 0; i < values.Length; i++) + { + if (array.IsValid(i)) + { + acc += values[i]; + } + } + return acc; + } + + /// <summary>Returns the smallest non-null element, or null if there are no non-null elements.</summary> + public static T? Min<T>(this PrimitiveArray<T> array) + where T : unmanaged, INumber<T>, IMinMaxValue<T> + { + if (array is null) throw new ArgumentNullException(nameof(array)); + + ReadOnlySpan<T> values = array.Values; + + if (values.Length == 0 || array.Length - array.NullCount == 0) + { + return null; + } + + if (array.NullCount == 0) + { + return TensorPrimitives.Min(values); + } + + T min = T.MaxValue; + for (int i = 0; i < values.Length; i++) + { + if (!array.IsValid(i)) continue; + if (values[i] < min) { min = values[i]; } + } + return min; Review Comment: In the null-aware path, initializing `min` to `T.MaxValue` can produce incorrect results for floating-point arrays containing NaN(s) (e.g., if the only non-null value is NaN, this returns `T.MaxValue`). It also makes net8.0+ semantics diverge from the netstandard/net462 implementation below, which seeds from the first valid element. Seed `min` from the first valid element instead of using a sentinel. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
