eerhardt commented on a change in pull request #9356:
URL: https://github.com/apache/arrow/pull/9356#discussion_r579334666
##########
File path: csharp/src/Apache.Arrow/Flatbuf/BodyCompressionMethod.cs
##########
@@ -0,0 +1,24 @@
+// <auto-generated>
+// automatically generated by the FlatBuffers compiler, do not modify
+// </auto-generated>
+
+namespace Apache.Arrow.Flatbuf
+{
+
+/// Provided for forward compatibility in case we need to support different
+/// strategies for compressing the IPC message body (like whole-body
+/// compression rather than buffer-level) in the future
+public enum BodyCompressionMethod : sbyte
Review comment:
These enums shouldn't be `public`. Nothing in `Flatbuf` should be public.
##########
File path: csharp/test/Apache.Arrow.Tests/ArrowReaderVerifier.cs
##########
@@ -147,6 +150,40 @@ private void CompareBinaryArrays<T>(BinaryArray
actualArray)
Assert.True(expectedArray.Values.Slice(0,
expectedArray.Length).SequenceEqual(actualArray.Values.Slice(0,
actualArray.Length)));
}
+ private void CompareArrays(Decimal128Array actualArray)
Review comment:
Can we combine these two methods into one for a `FixedSizeBinaryArray`?
##########
File path: csharp/src/Apache.Arrow/ArrowBuffer.Builder.cs
##########
@@ -211,6 +211,23 @@ public ArrowBuffer Build(MemoryAllocator allocator =
default)
return new ArrowBuffer(memoryOwner);
}
+ /// <summary>
+ /// Build an Arrow buffer from the appended contents so far of the
specified byte size.
+ /// </summary>
+ /// <param name="allocator">Optional memory allocator.</param>
+ /// <returns>Returns an <see cref="ArrowBuffer"/> object.</returns>
+ public ArrowBuffer Build(int byteSize, MemoryAllocator allocator =
default)
Review comment:
Does this need to be public? Are we expecting external callers to use it?
##########
File path: csharp/test/Apache.Arrow.Tests/TestData.cs
##########
@@ -48,8 +48,9 @@ public static RecordBatch CreateSampleRecordBatch(int length,
int columnSetCount
builder.Field(CreateField(TimestampType.Default, i));
builder.Field(CreateField(StringType.Default, i));
builder.Field(CreateField(new StructType(new List<Field> {
CreateField(StringType.Default, i), CreateField(Int32Type.Default, i) }), i));
+ builder.Field(CreateField(new Decimal128Type(20, 10), i));
+ builder.Field(CreateField(new Decimal256Type(20, 10), i));
Review comment:
Can we test with different precision and scales?
##########
File path: csharp/test/Apache.Arrow.Benchmarks/ArrowReaderBenchmark.cs
##########
@@ -116,6 +116,10 @@ private static double SumAllNumbers(RecordBatch
recordBatch)
DoubleArray doubleArray = (DoubleArray)array;
sum += Sum(doubleArray);
break;
+ case ArrowTypeId.Decimal128:
+ Decimal128Array decimalArray = (Decimal128Array)array;
+ sum += (double)Sum(decimalArray);
Review comment:
This cast is unnecessary since the `Sum` method already returns a
`double`.
##########
File path: csharp/src/Apache.Arrow/ArrowBuffer.Builder.cs
##########
@@ -211,6 +211,23 @@ public ArrowBuffer Build(MemoryAllocator allocator =
default)
return new ArrowBuffer(memoryOwner);
}
+ /// <summary>
+ /// Build an Arrow buffer from the appended contents so far of the
specified byte size.
+ /// </summary>
+ /// <param name="allocator">Optional memory allocator.</param>
+ /// <returns>Returns an <see cref="ArrowBuffer"/> object.</returns>
+ public ArrowBuffer Build(int byteSize, MemoryAllocator allocator =
default)
+ {
+ int currentBytesLength = Length * _size;
Review comment:
Can we refactor the above method to call into this method with `64`?
That way there is less duplicated code.
##########
File path: csharp/src/Apache.Arrow/Arrays/Decimal128Array.cs
##########
@@ -0,0 +1,96 @@
+// 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.Diagnostics;
+using System.Numerics;
+using Apache.Arrow.Arrays;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class Decimal128Array : FixedSizeBinaryArray
+ {
+ public class Builder : BuilderBase<Decimal128Array, Builder>
+ {
+ public Builder(Decimal128Type type) : base(type, 16)
+ {
+ DataType = type;
+ }
+
+ protected new Decimal128Type DataType { get; }
+
+ protected override Decimal128Array Build(ArrayData data)
+ {
+ return new Decimal128Array(data);
+ }
+
+ public Builder Append(decimal value)
+ {
+ DecimalUtility.CheckPrecisionAndScale(value,
DataType.Precision, DataType.Scale, out BigInteger integerValue);
+ byte[] bytes = DecimalUtility.GetBytes(integerValue,
DataType.ByteWidth);
+
+ return Append(bytes);
+ }
+
+ public Builder AppendRange(IEnumerable<decimal> values)
+ {
+ if (values == null)
+ {
+ throw new ArgumentNullException(nameof(values));
+ }
+
+ foreach (decimal d in values)
+ {
+ Append(d);
+ }
+
+ return Instance;
+ }
+
+ public Builder Set(int index, decimal value)
+ {
+ DecimalUtility.CheckPrecisionAndScale(value,
DataType.Precision, DataType.Scale, out BigInteger integerValue);
+ byte[] bytes = DecimalUtility.GetBytes(integerValue,
DataType.ByteWidth);
Review comment:
A different way that doesn't involve `stackalloc` would be to get a
`Span<byte>` directly into the `ValueBuffer` itself (after making sure it is
long enough) and passing that to `DecimalUtility.GetBytes`.
##########
File path: csharp/src/Apache.Arrow/DecimalUtility.cs
##########
@@ -0,0 +1,188 @@
+// 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 System.Numerics;
+using System.Runtime.InteropServices;
+
+namespace Apache.Arrow
+{
+ /// <summary>
+ /// This is semi-optimised best attempt at converting to / from decimal
and the buffers
+ /// </summary>
+ public static class DecimalUtility
Review comment:
Does this need to be public?
##########
File path: csharp/src/Apache.Arrow/Arrays/Decimal128Array.cs
##########
@@ -0,0 +1,96 @@
+// 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.Diagnostics;
+using System.Numerics;
+using Apache.Arrow.Arrays;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class Decimal128Array : FixedSizeBinaryArray
+ {
+ public class Builder : BuilderBase<Decimal128Array, Builder>
+ {
+ public Builder(Decimal128Type type) : base(type, 16)
+ {
+ DataType = type;
+ }
+
+ protected new Decimal128Type DataType { get; }
+
+ protected override Decimal128Array Build(ArrayData data)
+ {
+ return new Decimal128Array(data);
+ }
+
+ public Builder Append(decimal value)
+ {
+ DecimalUtility.CheckPrecisionAndScale(value,
DataType.Precision, DataType.Scale, out BigInteger integerValue);
+ byte[] bytes = DecimalUtility.GetBytes(integerValue,
DataType.ByteWidth);
+
+ return Append(bytes);
+ }
+
+ public Builder AppendRange(IEnumerable<decimal> values)
+ {
+ if (values == null)
+ {
+ throw new ArgumentNullException(nameof(values));
+ }
+
+ foreach (decimal d in values)
+ {
+ Append(d);
+ }
+
+ return Instance;
+ }
+
+ public Builder Set(int index, decimal value)
+ {
+ DecimalUtility.CheckPrecisionAndScale(value,
DataType.Precision, DataType.Scale, out BigInteger integerValue);
+ byte[] bytes = DecimalUtility.GetBytes(integerValue,
DataType.ByteWidth);
Review comment:
This is a bit of a performance trap, since it creates a new `byte[]`
every time it is called.
Instead, we should change this around so this method `stackalloc`s a buffer
for `DecimalUtility.GetBytes` to write into. It can then be passed as a
`Span<byte>` of length ByteWidth. Then the `Span<byte>` can be passed to `Set`,
which copies the bytes over. That way no memory is allocated on the heap.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
