CurtHagenlocher commented on code in PR #40189:
URL: https://github.com/apache/arrow/pull/40189#discussion_r1536656191
##########
csharp/src/Apache.Arrow/Arrays/BinaryArray.cs:
##########
@@ -253,14 +253,25 @@ public TBuilder Resize(int length)
public TBuilder Swap(int i, int j)
Review Comment:
I think most users would expect this to swap the ith and jth elements of the
binary array and not the ith and jth bytes of the underlying buffer -- or, at
least I would. That is, if my logical array were to look like `{null, {0x00,
0x01, 0x02}, {0xff}, {0xfe, 0xfd}}` then calling Swap(1, 2) should result in
`{null, {0xff}, {0x00, 0x01, 0x02}, {0xfe, 0xfd}}`. But with this change,
calling Swap(1, 2) would instead produce `{null, {0xff}, {0x01, 0x00, 0x02},
{0xfe, 0xfd}}`.
##########
csharp/src/Apache.Arrow/Arrays/BinaryArray.cs:
##########
@@ -253,14 +253,25 @@ public TBuilder Resize(int length)
public TBuilder Swap(int i, int j)
{
- // TODO: Implement
- throw new NotImplementedException();
+ byte ti = ValueBuffer.Span[i];
+ byte tj = ValueBuffer.Span[j];
+ ValueBuffer.Span[i] = tj;
+ ValueBuffer.Span[j] = ti;
+ return Instance;
}
public TBuilder Set(int index, byte value)
{
- // TODO: Implement
- throw new NotImplementedException();
+ ValueBuffer.Span[index] = value;
+ ValidityBuffer.Set(index);
+ return Instance;
+ }
+
+ public TBuilder SetNull(int index)
+ {
+ ValueBuffer.Span[index] = default;
+ ValidityBuffer.Set(index, false);
Review Comment:
The indexes in the validity buffer don't align with the indexes in the value
buffer unless each element of the binary array has a length of exactly one.
To implement SetNull correctly, if the previous value was non-null then it
would be necessary to compact the value buffer and adjust all of the offsets in
ValueOffsets.
##########
csharp/src/Apache.Arrow/Arrays/BinaryArray.cs:
##########
@@ -253,14 +253,25 @@ public TBuilder Resize(int length)
public TBuilder Swap(int i, int j)
{
- // TODO: Implement
- throw new NotImplementedException();
+ byte ti = ValueBuffer.Span[i];
+ byte tj = ValueBuffer.Span[j];
+ ValueBuffer.Span[i] = tj;
+ ValueBuffer.Span[j] = ti;
+ return Instance;
}
public TBuilder Set(int index, byte value)
{
- // TODO: Implement
- throw new NotImplementedException();
+ ValueBuffer.Span[index] = value;
+ ValidityBuffer.Set(index);
+ return Instance;
+ }
+
+ public TBuilder SetNull(int index)
+ {
+ ValueBuffer.Span[index] = default;
+ ValidityBuffer.Set(index, false);
Review Comment:
(The same comment roughly applies to the implementation of Set, above.)
--
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]
