[GitHub] [arrow] eerhardt commented on a change in pull request #7671: ARROW-8344: [C#] Bug-fixes to binary array plus other improvements
eerhardt commented on a change in pull request #7671:
URL: https://github.com/apache/arrow/pull/7671#discussion_r453045241
##
File path: csharp/src/Apache.Arrow/Arrays/BinaryArray.cs
##
@@ -66,87 +66,158 @@ protected BuilderBase(IArrowType dataType)
ValueOffsets = new ArrowBuffer.Builder();
ValueBuffer = new ArrowBuffer.Builder();
ValidityBuffer = new ArrowBuffer.BitmapBuilder();
+
+// From the docs:
+//
+// The offsets buffer contains length + 1 signed integers
(either 32-bit or 64-bit, depending on the
+// logical type), which encode the start position of each slot
in the data buffer. The length of the
+// value in each slot is computed using the difference between
the offset at that slot’s index and the
+// subsequent offset.
+//
+// In this builder, we choose to append the first offset
(zero) upon construction, and each trailing
+// offset is then added after each individual item has been
appended.
+ValueOffsets.Append(this.Offset);
}
protected abstract TArray Build(ArrayData data);
-public int Length => ValueOffsets.Length;
+///
+/// Gets the length of the array built so far.
+///
+public int Length => ValueOffsets.Length - 1;
+///
+/// Build an Arrow array from the appended contents so far.
+///
+/// Optional memory allocator.
+/// Returns an array of type .
public TArray Build(MemoryAllocator allocator = default)
{
-ValueOffsets.Append(Offset);
-
-ArrowBuffer validityBuffer = NullCount > 0
-? ValidityBuffer.Build(allocator)
-: ArrowBuffer.Empty;
-
-var data = new ArrayData(DataType, ValueOffsets.Length - 1,
NullCount, 0,
-new[] { validityBuffer, ValueOffsets.Build(allocator),
ValueBuffer.Build(allocator) });
+var bufs = new[]
+{
+NullCount > 0 ? ValidityBuffer.Build(allocator) :
ArrowBuffer.Empty,
+ValueOffsets.Build(allocator),
+ValueBuffer.Build(allocator),
+};
+var data = new ArrayData(
+DataType,
+length: ValueOffsets.Length - 1,
+NullCount,
+offset: 0,
+bufs);
return Build(data);
}
+///
+/// Append a single null value to the array.
+///
+/// Returns the builder (for fluent-style
composition).
public TBuilder AppendNull()
{
-ValueOffsets.Append(Offset);
+// Do not add to the value buffer in the case of a null.
+// Note that we do not need to increment the offset as a
result.
ValidityBuffer.Append(false);
+ValueOffsets.Append(Offset);
return Instance;
}
+///
+/// Appends a value, consisting of a single byte, to the array.
+///
+/// Byte value to append.
+/// Returns the builder (for fluent-style
composition).
public TBuilder Append(byte value)
{
-ValueOffsets.Append(Offset);
ValueBuffer.Append(value);
-Offset++;
ValidityBuffer.Append(true);
+Offset++;
+ValueOffsets.Append(Offset);
return Instance;
}
+///
+/// Append a value, consisting of a span of bytes, to the array.
+///
+///
+/// Note that a single value is added, which consists of
arbitrarily many bytes. If multiple values are
+/// to be added, use the method.
+///
+/// Span of bytes to add.
+/// Returns the builder (for fluent-style
composition).
public TBuilder Append(ReadOnlySpan span)
{
-ValueOffsets.Append(Offset);
ValueBuffer.Append(span);
ValidityBuffer.Append(true);
Offset += span.Length;
+ValueOffsets.Append(Offset);
return Instance;
}
-public TBuilder AppendRange(IEnumerable values)
+///
+/// Append a value, consisting of an enumerable collection of
bytes, to the array.
+///
+///
+/// Note that this method appends a single value, which may
consist of arbitrarily many bytes. If multiple
+
[GitHub] [arrow] eerhardt commented on a change in pull request #7671: ARROW-8344: [C#] Bug-fixes to binary array plus other improvements
eerhardt commented on a change in pull request #7671:
URL: https://github.com/apache/arrow/pull/7671#discussion_r453042194
##
File path: csharp/src/Apache.Arrow/Arrays/BinaryArray.cs
##
@@ -237,7 +329,9 @@ public ReadOnlySpan GetBytes(int index)
if (IsNull(index))
{
-return null;
+// Note that `return null;` is valid syntax, but would be
misleading as `null` in the context of a span
+// is actually returned as an empty span.
+return ReadOnlySpan.Empty;
Review comment:
I've been confused and bit by this a few times in different projects.
All the following ways produce an "equivalent" span:
* `return null` (this uses the implicit operator from an array)
* `return default`
* `return ReadOnlySpan.Empty` (this just says `return default`
underneath the covers)
I agree that `ReadOnlySpan.Empty` is the most clear thing to return here.
Next would be `return default`. `return null` is the worst of the three options
IMO.
##
File path: csharp/src/Apache.Arrow/Arrays/BinaryArray.cs
##
@@ -66,87 +66,158 @@ protected BuilderBase(IArrowType dataType)
ValueOffsets = new ArrowBuffer.Builder();
ValueBuffer = new ArrowBuffer.Builder();
ValidityBuffer = new ArrowBuffer.BitmapBuilder();
+
+// From the docs:
+//
+// The offsets buffer contains length + 1 signed integers
(either 32-bit or 64-bit, depending on the
+// logical type), which encode the start position of each slot
in the data buffer. The length of the
+// value in each slot is computed using the difference between
the offset at that slot’s index and the
+// subsequent offset.
+//
+// In this builder, we choose to append the first offset
(zero) upon construction, and each trailing
+// offset is then added after each individual item has been
appended.
+ValueOffsets.Append(this.Offset);
}
protected abstract TArray Build(ArrayData data);
-public int Length => ValueOffsets.Length;
+///
+/// Gets the length of the array built so far.
+///
+public int Length => ValueOffsets.Length - 1;
+///
+/// Build an Arrow array from the appended contents so far.
+///
+/// Optional memory allocator.
+/// Returns an array of type .
public TArray Build(MemoryAllocator allocator = default)
{
-ValueOffsets.Append(Offset);
-
-ArrowBuffer validityBuffer = NullCount > 0
-? ValidityBuffer.Build(allocator)
-: ArrowBuffer.Empty;
-
-var data = new ArrayData(DataType, ValueOffsets.Length - 1,
NullCount, 0,
-new[] { validityBuffer, ValueOffsets.Build(allocator),
ValueBuffer.Build(allocator) });
+var bufs = new[]
+{
+NullCount > 0 ? ValidityBuffer.Build(allocator) :
ArrowBuffer.Empty,
+ValueOffsets.Build(allocator),
+ValueBuffer.Build(allocator),
+};
+var data = new ArrayData(
+DataType,
+length: ValueOffsets.Length - 1,
Review comment:
Can this line just be `Length` ?
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
[GitHub] [arrow] eerhardt commented on a change in pull request #7671: ARROW-8344: [C#] Bug-fixes to binary array plus other improvements
eerhardt commented on a change in pull request #7671: URL: https://github.com/apache/arrow/pull/7671#discussion_r453038089 ## File path: csharp/src/Apache.Arrow/Arrays/BinaryArray.cs ## @@ -66,87 +66,158 @@ protected BuilderBase(IArrowType dataType) ValueOffsets = new ArrowBuffer.Builder(); ValueBuffer = new ArrowBuffer.Builder(); ValidityBuffer = new ArrowBuffer.BitmapBuilder(); + +// From the docs: +// +// The offsets buffer contains length + 1 signed integers (either 32-bit or 64-bit, depending on the +// logical type), which encode the start position of each slot in the data buffer. The length of the +// value in each slot is computed using the difference between the offset at that slot’s index and the +// subsequent offset. +// +// In this builder, we choose to append the first offset (zero) upon construction, and each trailing +// offset is then added after each individual item has been appended. +ValueOffsets.Append(this.Offset); Review comment: Nice fix. 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
