eerhardt commented on a change in pull request #10990: URL: https://github.com/apache/arrow/pull/10990#discussion_r714175901
########## File path: csharp/src/Apache.Arrow/Arrays/ArrayDataConcatenator.cs ########## @@ -0,0 +1,240 @@ +// 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 Apache.Arrow.Memory; +using Apache.Arrow.Types; +using System; +using System.Collections.Generic; + +namespace Apache.Arrow +{ + public static class ArrayDataConcatenator Review comment: Is it possible to mark the new classes as `internal`? Making them public means that external callers can call them, and we then need to maintain the public surface area (unless we want to make breaking changes). If this is only marked `public` for testing, instead, try as hard as you can to write tests using the `public` API - like a customer would. If that can't be done (since we can't write dictionaries in C# yet), consider either using InternalsVisibleTo or Reflection (preferred) to call these methods from the tests. ########## File path: csharp/src/Apache.Arrow/Arrays/ArrowArrayBuilderFactory.cs ########## @@ -18,9 +18,9 @@ namespace Apache.Arrow { - static class ArrowArrayBuilderFactory + public static class ArrowArrayBuilderFactory Review comment: Is it possible to not make this public API? Would normal callers need it? We should avoid making it public API just for testing. ########## File path: csharp/src/Apache.Arrow/Arrays/ArrayDataConcatenator.cs ########## @@ -0,0 +1,240 @@ +// 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 Apache.Arrow.Memory; +using Apache.Arrow.Types; +using System; +using System.Collections.Generic; + +namespace Apache.Arrow +{ + public static class ArrayDataConcatenator + { + public static ArrayData Concatenate(IReadOnlyList<ArrayData> arrayDataList, MemoryAllocator allocator = default) + { + if (arrayDataList == null || arrayDataList.Count == 0) + { + return null; + } + + if (arrayDataList.Count == 1) + { + return arrayDataList[0]; + } + + var arrowArrayConcatinateVisitor = new ArrayDataConcatinationVisitor(arrayDataList, allocator); + + IArrowType type = arrayDataList[0].DataType; + type.Accept(arrowArrayConcatinateVisitor); + + return arrowArrayConcatinateVisitor.Result; + } + + private class ArrayDataConcatinationVisitor : + IArrowTypeVisitor<BooleanType>, + IArrowTypeVisitor<FixedWidthType>, + IArrowTypeVisitor<BinaryType>, + IArrowTypeVisitor<StringType>, + IArrowTypeVisitor<ListType>, + IArrowTypeVisitor<StructType> + { + public ArrayData Result { get; private set; } + private IReadOnlyList<ArrayData> _arrayDataList; Review comment: ```suggestion private readonly IReadOnlyList<ArrayData> _arrayDataList; ``` ########## File path: csharp/src/Apache.Arrow/Arrays/ArrayDataConcatenator.cs ########## @@ -0,0 +1,240 @@ +// 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 Apache.Arrow.Memory; +using Apache.Arrow.Types; +using System; +using System.Collections.Generic; + +namespace Apache.Arrow +{ + public static class ArrayDataConcatenator + { + public static ArrayData Concatenate(IReadOnlyList<ArrayData> arrayDataList, MemoryAllocator allocator = default) + { + if (arrayDataList == null || arrayDataList.Count == 0) + { + return null; + } + + if (arrayDataList.Count == 1) + { + return arrayDataList[0]; + } + + var arrowArrayConcatinateVisitor = new ArrayDataConcatinationVisitor(arrayDataList, allocator); + + IArrowType type = arrayDataList[0].DataType; + type.Accept(arrowArrayConcatinateVisitor); + + return arrowArrayConcatinateVisitor.Result; + } + + private class ArrayDataConcatinationVisitor : + IArrowTypeVisitor<BooleanType>, + IArrowTypeVisitor<FixedWidthType>, + IArrowTypeVisitor<BinaryType>, + IArrowTypeVisitor<StringType>, + IArrowTypeVisitor<ListType>, + IArrowTypeVisitor<StructType> + { + public ArrayData Result { get; private set; } + private IReadOnlyList<ArrayData> _arrayDataList; + private readonly int _totalLength; + private readonly int _totalNullCount; + private readonly MemoryAllocator _allocator; + + public ArrayDataConcatinationVisitor(IReadOnlyList<ArrayData> arrayDataList, MemoryAllocator allocator = default) + { + _arrayDataList = arrayDataList; + _allocator = allocator; + + foreach (ArrayData arrayData in _arrayDataList) + { + _totalLength += arrayData.Length; + _totalNullCount += arrayData.NullCount; + } + } + + public void Visit(BooleanType type) + { + CheckData(type, 2); + ArrowBuffer validityBuffer = ConcatenateValidityBuffer(); + ArrowBuffer valueBuffer = ConcatenateBitmapBuffer(1); + + Result = new ArrayData(type, _totalLength, _totalNullCount, 0, new ArrowBuffer[] { validityBuffer, valueBuffer }); + } + + public void Visit(FixedWidthType type) + { + CheckData(type, 2); + ArrowBuffer validityBuffer = ConcatenateValidityBuffer(); + ArrowBuffer valueBuffer = ConcatenateFixedWidthTypeValueBuffer(type); + + Result = new ArrayData(type, _totalLength, _totalNullCount, 0, new ArrowBuffer[] { validityBuffer, valueBuffer }); + } + + public void Visit(BinaryType type) => ConcateneteVariableBinaryArrayData(type); + + public void Visit(StringType type) => ConcateneteVariableBinaryArrayData(type); + + public void Visit(ListType type) + { + CheckData(type, 2); + ArrowBuffer validityBuffer = ConcatenateValidityBuffer(); + ArrowBuffer offsetBuffer = ConcateneteOffsetBuffer(); + ArrayData child = Concatenate(SelectChildren(0), _allocator); + + Result = new ArrayData(type, _totalLength, _totalNullCount, 0, new ArrowBuffer[] { validityBuffer, offsetBuffer }, new[] { child }); + } + + public void Visit(StructType type) + { + CheckData(type, 1); + List<ArrayData> children = new List<ArrayData>(type.Fields.Count); + + for (int i = 0; i < type.Fields.Count; i++) + { + children.Add(Concatenate(SelectChildren(i), _allocator)); + } + + Result = new ArrayData(type, _arrayDataList[0].Length, _arrayDataList[0].NullCount, 0, _arrayDataList[0].Buffers, children); + } + + public void Visit(IArrowType type) + { + throw new NotImplementedException($"Concatination for {type.Name} is not supported yet."); + } + + private void CheckData(IArrowType type, int expectedBufferCount) + { + foreach (ArrayData arrayData in _arrayDataList) + { + arrayData.EnsureDataType(type.TypeId); + arrayData.EnsureBufferCount(expectedBufferCount); + } + } + + private void ConcateneteVariableBinaryArrayData(IArrowType type) + { + CheckData(type, 3); + ArrowBuffer validityBuffer = ConcatenateValidityBuffer(); + ArrowBuffer offsetBuffer = ConcateneteOffsetBuffer(); + ArrowBuffer valueBuffer = ConcatenateVariableBinaryValueBuffer(); + + Result = new ArrayData(type, _totalLength, _totalNullCount, 0, new ArrowBuffer[] { validityBuffer, offsetBuffer, valueBuffer }); + } + + private ArrowBuffer ConcatenateValidityBuffer() + { + if (_totalNullCount == 0) + { + return ArrowBuffer.Empty; + } + + return ConcatenateBitmapBuffer(0); + } + + private ArrowBuffer ConcatenateBitmapBuffer(int bufferIndex) + { + var builder = new ArrowBuffer.BitmapBuilder(_totalLength); + + foreach (ArrayData arrayData in _arrayDataList) + { + int length = arrayData.Length; + ReadOnlySpan<byte> span = arrayData.Buffers[bufferIndex].Span; + + for (int i = 0; i < length; i++) + { + builder.Append(span.IsEmpty || BitUtility.GetBit(span, i)); + } + } + + return builder.Build(_allocator); + } + + private ArrowBuffer ConcatenateFixedWidthTypeValueBuffer(FixedWidthType type) + { + int typeByteWidth = type.BitWidth / 8; + var builder = new ArrowBuffer.Builder<byte>(_totalLength * typeByteWidth); + + foreach (ArrayData arrayData in _arrayDataList) + { + int length = arrayData.Length; + int byteLength = length * typeByteWidth; + + builder.Append(arrayData.Buffers[1].Span.Slice(0, byteLength)); + } + + return builder.Build(_allocator); + } + + private ArrowBuffer ConcatenateVariableBinaryValueBuffer() + { + var builder = new ArrowBuffer.Builder<byte>(); + + foreach (ArrayData arrayData in _arrayDataList) + { + int lastOffset = arrayData.Buffers[1].Span.CastTo<int>()[arrayData.Length]; + builder.Append(arrayData.Buffers[2].Span.Slice(0, lastOffset)); + } + + return builder.Build(_allocator); + } + + private ArrowBuffer ConcateneteOffsetBuffer() + { + var builder = new ArrowBuffer.Builder<int>(_totalLength + 1); + int baseOffset = 0; + + builder.Append(0); + + foreach (ArrayData arrayData in _arrayDataList) + { + if (arrayData.Length == 0) + { + continue; + } + + // The first offset is always 0. + // It should be skipped because it duplicate to the last offset of builder. + ReadOnlySpan<int> span = arrayData.Buffers[1].Span.CastTo<int>().Slice(1, arrayData.Length); Review comment: Doesn't this need to be `arrayData.Length - 1`? Because you sliced off the first element. ```suggestion ReadOnlySpan<int> span = arrayData.Buffers[1].Span.CastTo<int>().Slice(1, arrayData.Length - 1); ``` ########## File path: csharp/src/Apache.Arrow/Arrays/ArrayDataConcatenator.cs ########## @@ -0,0 +1,240 @@ +// 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 Apache.Arrow.Memory; +using Apache.Arrow.Types; +using System; +using System.Collections.Generic; + +namespace Apache.Arrow +{ + public static class ArrayDataConcatenator + { + public static ArrayData Concatenate(IReadOnlyList<ArrayData> arrayDataList, MemoryAllocator allocator = default) + { + if (arrayDataList == null || arrayDataList.Count == 0) + { + return null; + } + + if (arrayDataList.Count == 1) + { + return arrayDataList[0]; + } + + var arrowArrayConcatinateVisitor = new ArrayDataConcatinationVisitor(arrayDataList, allocator); + + IArrowType type = arrayDataList[0].DataType; + type.Accept(arrowArrayConcatinateVisitor); + + return arrowArrayConcatinateVisitor.Result; + } + + private class ArrayDataConcatinationVisitor : + IArrowTypeVisitor<BooleanType>, + IArrowTypeVisitor<FixedWidthType>, + IArrowTypeVisitor<BinaryType>, + IArrowTypeVisitor<StringType>, + IArrowTypeVisitor<ListType>, + IArrowTypeVisitor<StructType> + { + public ArrayData Result { get; private set; } + private IReadOnlyList<ArrayData> _arrayDataList; + private readonly int _totalLength; + private readonly int _totalNullCount; + private readonly MemoryAllocator _allocator; + + public ArrayDataConcatinationVisitor(IReadOnlyList<ArrayData> arrayDataList, MemoryAllocator allocator = default) + { + _arrayDataList = arrayDataList; + _allocator = allocator; + + foreach (ArrayData arrayData in _arrayDataList) + { + _totalLength += arrayData.Length; + _totalNullCount += arrayData.NullCount; + } + } + + public void Visit(BooleanType type) + { + CheckData(type, 2); + ArrowBuffer validityBuffer = ConcatenateValidityBuffer(); + ArrowBuffer valueBuffer = ConcatenateBitmapBuffer(1); + + Result = new ArrayData(type, _totalLength, _totalNullCount, 0, new ArrowBuffer[] { validityBuffer, valueBuffer }); + } + + public void Visit(FixedWidthType type) + { + CheckData(type, 2); + ArrowBuffer validityBuffer = ConcatenateValidityBuffer(); + ArrowBuffer valueBuffer = ConcatenateFixedWidthTypeValueBuffer(type); + + Result = new ArrayData(type, _totalLength, _totalNullCount, 0, new ArrowBuffer[] { validityBuffer, valueBuffer }); + } + + public void Visit(BinaryType type) => ConcateneteVariableBinaryArrayData(type); + + public void Visit(StringType type) => ConcateneteVariableBinaryArrayData(type); + + public void Visit(ListType type) + { + CheckData(type, 2); + ArrowBuffer validityBuffer = ConcatenateValidityBuffer(); + ArrowBuffer offsetBuffer = ConcateneteOffsetBuffer(); + ArrayData child = Concatenate(SelectChildren(0), _allocator); + + Result = new ArrayData(type, _totalLength, _totalNullCount, 0, new ArrowBuffer[] { validityBuffer, offsetBuffer }, new[] { child }); + } + + public void Visit(StructType type) + { + CheckData(type, 1); + List<ArrayData> children = new List<ArrayData>(type.Fields.Count); + + for (int i = 0; i < type.Fields.Count; i++) + { + children.Add(Concatenate(SelectChildren(i), _allocator)); + } + + Result = new ArrayData(type, _arrayDataList[0].Length, _arrayDataList[0].NullCount, 0, _arrayDataList[0].Buffers, children); + } + + public void Visit(IArrowType type) + { + throw new NotImplementedException($"Concatination for {type.Name} is not supported yet."); + } + + private void CheckData(IArrowType type, int expectedBufferCount) + { + foreach (ArrayData arrayData in _arrayDataList) + { + arrayData.EnsureDataType(type.TypeId); + arrayData.EnsureBufferCount(expectedBufferCount); + } + } + + private void ConcateneteVariableBinaryArrayData(IArrowType type) Review comment: ```suggestion private void ConcatenateVariableBinaryArrayData(IArrowType type) ``` ########## File path: csharp/src/Apache.Arrow/Arrays/ArrayDataConcatenator.cs ########## @@ -0,0 +1,240 @@ +// 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 Apache.Arrow.Memory; +using Apache.Arrow.Types; +using System; +using System.Collections.Generic; + +namespace Apache.Arrow +{ + public static class ArrayDataConcatenator + { + public static ArrayData Concatenate(IReadOnlyList<ArrayData> arrayDataList, MemoryAllocator allocator = default) + { + if (arrayDataList == null || arrayDataList.Count == 0) + { + return null; + } + + if (arrayDataList.Count == 1) + { + return arrayDataList[0]; + } + + var arrowArrayConcatinateVisitor = new ArrayDataConcatinationVisitor(arrayDataList, allocator); Review comment: ```suggestion var arrowArrayConcatenationVisitor = new ArrayDataConcatenationVisitor(arrayDataList, allocator); ``` ########## File path: csharp/test/Apache.Arrow.Tests/ArrowArrayConcatenatorTests.cs ########## @@ -0,0 +1,383 @@ +// 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.Collections.Generic; +using System.Linq; +using Apache.Arrow.Types; +using Xunit; + +namespace Apache.Arrow.Tests +{ + public class ArrowArrayConcatenatorTests + { + [Fact] + public void TestStandardCases() + { + foreach ((List<IArrowArray> testTargetArrayList, IArrowArray expectedArray) in GenerateTestData()) + { + IArrowArray actualArray = ArrowArrayConcatenator.Concatenate(testTargetArrayList); + ArrowReaderVerifier.CompareArrays(expectedArray, actualArray); + } + } + + [Fact] + public void TestNullOrEmpty() + { + Assert.Null(ArrowArrayConcatenator.Concatenate(null)); + Assert.Null(ArrowArrayConcatenator.Concatenate(new List<IArrowArray>())); + } + + [Fact] + public void TestSingleElement() + { + Int32Array array = new Int32Array.Builder().Append(1).Append(2).Build(); + IArrowArray actualArray = ArrowArrayConcatenator.Concatenate(new [] { array }); + ArrowReaderVerifier.CompareArrays(array, actualArray); + } + + private static IEnumerable<Tuple<List<IArrowArray>, IArrowArray>> GenerateTestData() + { + var targetTypes = new List<IArrowType>() { + BooleanType.Default, + Int8Type.Default, + Int16Type.Default, + Int32Type.Default, + Int64Type.Default, + UInt8Type.Default, + UInt16Type.Default, + UInt32Type.Default, + UInt64Type.Default, + FloatType.Default, + DoubleType.Default, + BinaryType.Default, + StringType.Default, + Date32Type.Default, + Date64Type.Default, + TimestampType.Default, + new Decimal128Type(14, 10), + new Decimal256Type(14,10), + new ListType(Int64Type.Default), + new StructType(new List<Field>{ + new Field.Builder().Name("Strings").DataType(StringType.Default).Nullable(true).Build(), + new Field.Builder().Name("Ints").DataType(Int32Type.Default).Nullable(true).Build() + }), + }; + + foreach (IArrowType type in targetTypes) + { + var creator = new TestDataGenerator(); + type.Accept(creator); + yield return Tuple.Create(creator.TestTargetArrayList, creator.ExpectedArray); + } + } + + private class TestDataGenerator : + IArrowTypeVisitor<BooleanType>, + IArrowTypeVisitor<Int8Type>, + IArrowTypeVisitor<Int16Type>, + IArrowTypeVisitor<Int32Type>, + IArrowTypeVisitor<Int64Type>, + IArrowTypeVisitor<UInt8Type>, + IArrowTypeVisitor<UInt16Type>, + IArrowTypeVisitor<UInt32Type>, + IArrowTypeVisitor<UInt64Type>, + IArrowTypeVisitor<FloatType>, + IArrowTypeVisitor<DoubleType>, + IArrowTypeVisitor<BinaryType>, + IArrowTypeVisitor<StringType>, + IArrowTypeVisitor<Decimal128Type>, + IArrowTypeVisitor<Decimal256Type>, + IArrowTypeVisitor<Date32Type>, + IArrowTypeVisitor<Date64Type>, + IArrowTypeVisitor<TimestampType>, + IArrowTypeVisitor<ListType>, + IArrowTypeVisitor<StructType> + { + + private List<List<int?>> _baseData; + + private int _baseDataListCount; + + private int _baseDataTotalElementCount; + + public List<IArrowArray> TestTargetArrayList { get; } + public IArrowArray ExpectedArray { get; private set; } + + public TestDataGenerator() + { + _baseData = new List<List<int?>> { + new List<int?> { 1, 2, 3 }, + new List<int?> { 100, 101, null }, + new List<int?> { 11, null, 12 }, + }; + + _baseDataListCount = _baseData.Count; + _baseDataTotalElementCount = _baseData.Sum(_ => _.Count); + TestTargetArrayList = new List<IArrowArray>(_baseDataListCount); + } + + public void Visit(BooleanType type) => GenerateTestData<bool, BooleanArray, BooleanArray.Builder>(type, x => x % 2 == 0); + public void Visit(Int8Type type) => GenerateTestData<sbyte, Int8Array, Int8Array.Builder>(type, x => (sbyte)x); + public void Visit(Int16Type type) => GenerateTestData<short, Int16Array, Int16Array.Builder>(type, x => (short)x); + public void Visit(Int32Type type) => GenerateTestData<int, Int32Array, Int32Array.Builder>(type, x => x); + public void Visit(Int64Type type) => GenerateTestData<long, Int64Array, Int64Array.Builder>(type, x => x); + public void Visit(UInt8Type type) => GenerateTestData<byte, UInt8Array, UInt8Array.Builder>(type, x => (byte)x); + public void Visit(UInt16Type type) => GenerateTestData<ushort, UInt16Array, UInt16Array.Builder>(type, x => (ushort)x); + public void Visit(UInt32Type type) => GenerateTestData<uint, UInt32Array, UInt32Array.Builder>(type, x => (uint)x); + public void Visit(UInt64Type type) => GenerateTestData<ulong, UInt64Array, UInt64Array.Builder>(type, x => (ulong)x); + public void Visit(FloatType type) => GenerateTestData<float, FloatArray, FloatArray.Builder>(type, x => x); + public void Visit(DoubleType type) => GenerateTestData<double, DoubleArray, DoubleArray.Builder>(type, x => x); + public void Visit(Date32Type type) => GenerateTestData<DateTime, Date32Array, Date32Array.Builder>(type, x => DateTime.MinValue.AddDays(x)); + public void Visit(Date64Type type) => GenerateTestData<DateTime, Date64Array, Date64Array.Builder>(type, x => DateTime.MinValue.AddDays(x)); Review comment: Can we reuse the existing code in the existing `TestData` class? -- 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]
