[GitHub] [arrow] eerhardt commented on a change in pull request #7158: ARROW-8788: [C#] Introduce bit-packed builder for null support in builders
eerhardt commented on a change in pull request #7158:
URL: https://github.com/apache/arrow/pull/7158#discussion_r437755214
##
File path: csharp/test/Apache.Arrow.Tests/ArrowBufferBitmapBuilderTests.cs
##
@@ -0,0 +1,496 @@
+// 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.
+
+namespace Apache.Arrow.Tests
+{
+using System;
Review comment:
(nit) These should go outside the namespace.
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 #7158: ARROW-8788: [C#] Introduce bit-packed builder for null support in builders
eerhardt commented on a change in pull request #7158:
URL: https://github.com/apache/arrow/pull/7158#discussion_r428800650
##
File path: csharp/src/Apache.Arrow/ArrowBuffer.Builder.cs
##
@@ -80,27 +140,59 @@ public Builder AppendRange(IEnumerable values)
return this;
}
-public Builder Reserve(int capacity)
+///
+/// Reserve a given number of items' additional capacity.
+///
+/// Number of items of required
additional capacity.
+/// Returns the builder (for fluent-style
composition).
+public Builder Reserve(int additionalCapacity)
{
-EnsureCapacity(capacity);
+if (additionalCapacity < 0)
+{
+throw new
ArgumentOutOfRangeException(nameof(additionalCapacity));
+}
+
+EnsureAdditionalCapacity(additionalCapacity);
return this;
}
+///
+/// Resize the buffer to a given size.
+///
+///
+/// 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.
+///
+///
+/// Note also that a negative capacity will result in the buffer
being resized to zero.
+///
+/// Number of items of required
capacity.
+/// Returns the builder (for fluent-style
composition).
public Builder Resize(int capacity)
{
+capacity = capacity < 0 ? 0 : capacity;
Review comment:
Can you open a new JIRA issue for this?
Also tagging @chutchinson, as the original author of the code. Any thoughts
here @chutchinson?
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 #7158: ARROW-8788: [C#] Introduce bit-packed builder for null support in builders
eerhardt commented on a change in pull request #7158:
URL: https://github.com/apache/arrow/pull/7158#discussion_r428797705
##
File path: csharp/test/Apache.Arrow.Tests/BooleanArrayTests.cs
##
@@ -18,6 +18,8 @@
namespace Apache.Arrow.Tests
{
+using System.Linq;
Review comment:
We should add an `.editorconfig` file in this repo, so VS will
automatically follow the formatting conventions used.
I've opened https://issues.apache.org/jira/browse/ARROW-8882 for this.
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 #7158: ARROW-8788: [C#] Introduce bit-packed builder for null support in builders
eerhardt commented on a change in pull request #7158:
URL: https://github.com/apache/arrow/pull/7158#discussion_r428142187
##
File path: csharp/test/Apache.Arrow.Tests/BooleanArrayTests.cs
##
@@ -18,6 +18,8 @@
namespace Apache.Arrow.Tests
{
+using System.Linq;
Review comment:
This should go at the top of the file with the rest of the usings.
##
File path: csharp/src/Apache.Arrow/ArrowBuffer.Builder.cs
##
@@ -22,51 +22,111 @@ namespace Apache.Arrow
{
public partial struct ArrowBuffer
{
+///
+/// The class is able to append value-type
items, with fluent-style methods, to build
+/// up an of contiguous items.
+///
+///
+/// Note that is not supported as a generic type
argument for this class. Please use
+/// instead.
+///
+/// Value-type of item to build into a
buffer.
public class Builder
where T : struct
{
private const int DefaultCapacity = 8;
private readonly int _size;
+///
+/// Gets the number of items of current capacity.
+///
public int Capacity => Memory.Length / _size;
+
+///
+/// Gets the number of items currently appended.
+///
public int Length { get; private set; }
+
+///
+/// Gets the raw byte memory underpinning the builder.
+///
public Memory Memory { get; private set; }
+
+///
+/// Gets the span of memory underpinning the builder.
+///
public Span Span
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => Memory.Span.CastTo();
}
+///
+/// Creates an instance of the class.
+///
+/// Number of items of initial capacity to
reserve.
public Builder(int capacity = DefaultCapacity)
{
+// Using `bool` as the template argument, if used in an
unrestricted fashion, would result in a buffer
+// with inappropriate contents being produced. Because C#
does not support template specialisation,
+// and because generic type constraints do not support
negation, we will throw a runtime error to
+// indicate that such a template type is not supported.
+if (typeof(T) == typeof(bool))
+{
+throw new ArgumentException(
+$"An instance of {nameof(Builder)} cannot be
instantiated, as `bool` is not an " +
+$"appropriate generic type to use with this class -
please use {nameof(BitPackedBuilder)} " +
+$"instead");
+}
Review comment:
I think this approach is fine. We do similar things in places in the
runtime, for example:
https://github.com/dotnet/runtime/blob/49f59a3cb935523c07187ce26bb03c7fa7fd66bd/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs#L349
The only thing I would change here is to throw a `NotSupportedException`
instead of an `ArgumentException`. `ArgumentException`s are for when an
argument passed to the method is invalid. But here it is the generic type.
##
File path: csharp/src/Apache.Arrow/ArrowBuffer.Builder.cs
##
@@ -22,51 +22,111 @@ namespace Apache.Arrow
{
public partial struct ArrowBuffer
{
+///
+/// The class is able to append value-type
items, with fluent-style methods, to build
+/// up an of contiguous items.
+///
+///
+/// Note that is not supported as a generic type
argument for this class. Please use
+/// instead.
+///
+/// Value-type of item to build into a
buffer.
public class Builder
where T : struct
{
private const int DefaultCapacity = 8;
private readonly int _size;
+///
+/// Gets the number of items of current capacity.
Review comment:
I think I'd borrow from StringBuilder's comments here:
https://docs.microsoft.com/en-us/dotnet/api/system.text.stringbuilder.capacity?view=netcore-3.1
> Gets the maximum number of items that can be contained in the memory
allocated by the current instance.
##
File path: csharp/src/Apache.Arrow/ArrowBuffer.Builder.cs
##
@@ -80,27 +140,59 @@ public Builder AppendRange(IEnumerable values)
return this;
}
-public Builder Reserve(int capacity)
+///
+/// Reserve a given number of items' additional capacity.
+///
+/// Number of items of required
additional capacity.
+/// Returns the builder (for fluent-style
composition).
+
