CurtHagenlocher commented on code in PR #36134:
URL: https://github.com/apache/arrow/pull/36134#discussion_r1396675290
##########
csharp/src/Apache.Arrow/Arrays/BinaryArray.cs:
##########
@@ -258,8 +259,77 @@ public TBuilder Swap(int i, int j)
public TBuilder Set(int index, byte value)
{
- // TODO: Implement
+ ValueBuffer.Span[index] = value;
+ return Instance;
+ }
+
+ public TBuilder SetNull(int offset)
Review Comment:
So this is nulling out the original bytes of the value but not freeing the
space it uses?
##########
csharp/src/Apache.Arrow/Arrays/BinaryArray.cs:
##########
@@ -258,8 +259,77 @@ public TBuilder Swap(int i, int j)
public TBuilder Set(int index, byte value)
{
- // TODO: Implement
+ ValueBuffer.Span[index] = value;
+ return Instance;
+ }
+
+ public TBuilder SetNull(int offset)
+ {
+ int index = ValueOffsets.Span[offset];
+ int length = GetOffsetValueLength(offset);
+
+ for (int i = 0; i < length; i++)
+ {
+ ValueBuffer.Span[index + i] = 0;
+ }
+
+ ValidityBuffer.Set(offset, false);
+
+ return Instance;
+ }
+
+ public TBuilder Set(int offset, ReadOnlySpan<byte> values)
+ {
+#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER
+ byte[] newValues = values.ToArray();
Review Comment:
It shouldn't be necessary to allocate temporary memory here, should it?
##########
csharp/src/Apache.Arrow/Arrays/BinaryArray.cs:
##########
@@ -258,8 +259,77 @@ public TBuilder Swap(int i, int j)
public TBuilder Set(int index, byte value)
{
- // TODO: Implement
+ ValueBuffer.Span[index] = value;
Review Comment:
I know this is a pre-existing function, but I don't understand what the
original author expected it to accomplish. Unless you can elucidate a use case
for it, I would consider keeping the original implementation and marking it as
obsolete.
##########
csharp/src/Apache.Arrow/Arrays/BinaryArray.cs:
##########
@@ -258,8 +259,77 @@ public TBuilder Swap(int i, int j)
public TBuilder Set(int index, byte value)
{
- // TODO: Implement
+ ValueBuffer.Span[index] = value;
+ return Instance;
+ }
+
+ public TBuilder SetNull(int offset)
+ {
+ int index = ValueOffsets.Span[offset];
+ int length = GetOffsetValueLength(offset);
+
+ for (int i = 0; i < length; i++)
+ {
+ ValueBuffer.Span[index + i] = 0;
+ }
+
+ ValidityBuffer.Set(offset, false);
+
+ return Instance;
+ }
+
+ public TBuilder Set(int offset, ReadOnlySpan<byte> values)
+ {
+#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER
Review Comment:
Nit: these are redundant in that NETSTANDARD2_1_OR_GREATER is always true if
NET6_0_OR_GREATER is also true.
Rather than throwing an exception when the functionality can't be supported,
I think it's better to simply not expose the function on downlevel .NET.
Otherwise, the experience is that someone discovers a method via Intellisense,
uses it and it always fails.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
