mr-smidge commented on a change in pull request #7671:
URL: https://github.com/apache/arrow/pull/7671#discussion_r453690533



##########
File path: csharp/src/Apache.Arrow/Arrays/BinaryArray.cs
##########
@@ -66,87 +66,158 @@ protected BuilderBase(IArrowType dataType)
                 ValueOffsets = new ArrowBuffer.Builder<int>();
                 ValueBuffer = new ArrowBuffer.Builder<byte>();
                 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;
+            /// <summary>
+            /// Gets the length of the array built so far.
+            /// </summary>
+            public int Length => ValueOffsets.Length - 1;
 
+            /// <summary>
+            /// Build an Arrow array from the appended contents so far.
+            /// </summary>
+            /// <param name="allocator">Optional memory allocator.</param>
+            /// <returns>Returns an array of type <typeparamref 
name="TArray"/>.</returns>
             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);
             }
 
+            /// <summary>
+            /// Append a single null value to the array.
+            /// </summary>
+            /// <returns>Returns the builder (for fluent-style 
composition).</returns>
             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;
             }
 
+            /// <summary>
+            /// Appends a value, consisting of a single byte, to the array.
+            /// </summary>
+            /// <param name="value">Byte value to append.</param>
+            /// <returns>Returns the builder (for fluent-style 
composition).</returns>
             public TBuilder Append(byte value)
             {
-                ValueOffsets.Append(Offset);
                 ValueBuffer.Append(value);
-                Offset++;
                 ValidityBuffer.Append(true);
+                Offset++;
+                ValueOffsets.Append(Offset);
                 return Instance;
             }
 
+            /// <summary>
+            /// Append a value, consisting of a span of bytes, to the array.
+            /// </summary>
+            /// <remarks>
+            /// Note that a single value is added, which consists of 
arbitrarily many bytes.  If multiple values are
+            /// to be added, use the <see cref="AppendRange"/> method.
+            /// </remarks>
+            /// <param name="span">Span of bytes to add.</param>
+            /// <returns>Returns the builder (for fluent-style 
composition).</returns>
             public TBuilder Append(ReadOnlySpan<byte> span)
             {
-                ValueOffsets.Append(Offset);
                 ValueBuffer.Append(span);
                 ValidityBuffer.Append(true);
                 Offset += span.Length;
+                ValueOffsets.Append(Offset);
                 return Instance;
             }
 
-            public TBuilder AppendRange(IEnumerable<byte[]> values)
+            /// <summary>
+            /// Append a value, consisting of an enumerable collection of 
bytes, to the array.
+            /// </summary>
+            /// <remarks>
+            /// Note that this method appends a single value, which may 
consist of arbitrarily many bytes.  If multiple
+            /// values are to be added, use the <see 
cref="AppendRange(IEnumerable{byte})"/> method instead.
+            /// </remarks>
+            /// <param name="value">Enumerable collection of bytes to 
add.</param>
+            /// <returns>Returns the builder (for fluent-style 
composition).</returns>
+            public TBuilder Append(IEnumerable<byte> value)
             {
-                foreach (byte[] arr in values)
+                if (value == null)
                 {
-                    if (arr == null)
-                    {
-                        AppendNull();
-                        continue;
-                    }
-                    int len = ValueBuffer.Length;
-                    ValueOffsets.Append(Offset);
-                    ValueBuffer.Append(arr);
-                    ValidityBuffer.Append(true);
-                    Offset += ValueBuffer.Length - len;
+                    return AppendNull();
                 }
 
+                // Note: by looking at the length of the value buffer before 
and after, we avoid having to iterate
+                // through the enumerable multiple times to get both length 
and contents.
+                int priorLength = ValueBuffer.Length;
+                ValueBuffer.AppendRange(value);
+                int valueLength = ValueBuffer.Length - priorLength;
+                Offset += valueLength;
+                ValidityBuffer.Append(true);
+                ValueOffsets.Append(Offset);
                 return Instance;
             }
 
+            /// <summary>
+            /// Append an enumerable collection of single-byte values to the 
array.
+            /// </summary>
+            /// <remarks>
+            /// Note that this method appends multiple values, each of which 
is a single byte.  If a single value is
+            /// to be added, use the <see cref="Append(IEnumerable{byte})"/> 
method instead.
+            /// </remarks>
+            /// <param name="values">Single-byte values to add.</param>
+            /// <returns>Returns the builder (for fluent-style 
composition).</returns>
             public TBuilder AppendRange(IEnumerable<byte> values)
             {
-                if (values == null)
+                foreach (byte b in values)
                 {
-                    return AppendNull();
+                    Append(b);
                 }
-                int len = ValueBuffer.Length;
-                ValueBuffer.AppendRange(values);
-                int valOffset = ValueBuffer.Length - len;
-                ValueOffsets.Append(Offset);
-                Offset += valOffset;
-                ValidityBuffer.Append(true);
+
+                return Instance;
+            }
+
+            /// <summary>
+            /// Append an enumerable collection of values to the array.
+            /// </summary>
+            /// <param name="values">Values to add.</param>
+            /// <returns>Returns the builder (for fluent-style 
composition).</returns>
+            public TBuilder AppendRange(IEnumerable<byte[]> values)
+            {
+                foreach (byte[] arr in values)

Review comment:
       Fixed.




----------------------------------------------------------------
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


Reply via email to