[GitHub] [arrow] chutchinson commented on a change in pull request #7158: ARROW-8788: [C#] Introduce bit-packed builder for null support in builders

[GitBox <gi...@apache.org>]

chutchinson commented on a change in pull request #7158:
URL: https://github.com/apache/arrow/pull/7158#discussion_r437589084



##########
File path: csharp/src/Apache.Arrow/ArrowBuffer.Builder.cs
##########
@@ -80,27 +140,59 @@ public Builder<T> AppendRange(IEnumerable<T> values)
                 return this;
             }
 
-            public Builder<T> Reserve(int capacity)
+            /// <summary>
+            /// Reserve a given number of items' additional capacity.
+            /// </summary>
+            /// <param name="additionalCapacity">Number of items of required additional capacity.</param>
+            /// <returns>Returns the builder (for fluent-style composition).</returns>
+            public Builder<T> Reserve(int additionalCapacity)
             {
-                EnsureCapacity(capacity);
+                if (additionalCapacity < 0)
+                {
+                    throw new ArgumentOutOfRangeException(nameof(additionalCapacity));
+                }
+
+                EnsureAdditionalCapacity(additionalCapacity);
                 return this;
             }
 
+            /// <summary>
+            /// Resize the buffer to a given size.
+            /// </summary>
+            /// <remarks>
+            /// Note that if the required capacity is smaller than the current length of the populated buffer so far,
+            /// the buffer will be truncated and items at the end of the buffer will be lost.
+            /// </remarks>
+            /// <remarks>
+            /// Note also that a negative capacity will result in the buffer being resized to zero.
+            /// </remarks>
+            /// <param name="capacity">Number of items of required capacity.</param>
+            /// <returns>Returns the builder (for fluent-style composition).</returns>
             public Builder<T> Resize(int capacity)
             {
+                capacity = capacity < 0 ? 0 : capacity;

Review comment:
       It's hard to say why I chose this behavior; this was written over a year ago. I can only imagine my thought process was that the capacity "can't go any lower", and avoiding the costs that come with throwing an exception.
   
   The unit test existed to verify the behavior as written, not necessarily to satisfy requirements, which I would agree was misguided.
   
   My current stance would be to match the behavior of the more mature implementations, and throw **ArgumentOutOfRangeException**. I tend to prefer avoiding exceptions and bounds checking where possible, but as a consumer I could understand being surprised to find that passing a negative capacity results in an empty buffer. I'm not sure I would expect this method in a situation where the bounds checking or exception stack unwinding would be harmful enough to be undesirable.
   
   




----------------------------------------------------------------
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:
users@infra.apache.org