Copilot commented on code in PR #297:
URL: https://github.com/apache/arrow-dotnet/pull/297#discussion_r3005501476
##########
src/Apache.Arrow/Column.cs:
##########
@@ -62,6 +64,35 @@ public Column Slice(int offset)
return new Column(Field, Data.Slice(offset));
}
+ /// <summary>
+ /// Slice this column with shared ownership. The returned slice keeps
the
+ /// underlying buffers alive via reference counting. The caller must
+ /// dispose the returned column when done.
+ /// </summary>
+ public Column SliceShared(int offset, int length)
+ {
+ return new Column(Field, Data.SliceShared(offset, length),
disposeArrayData: true);
+ }
+
+ /// <summary>
+ /// Slice this column with shared ownership. The returned slice keeps
the
+ /// underlying buffers alive via reference counting. The caller must
+ /// dispose the returned column when done.
+ /// </summary>
+ public Column SliceShared(int offset)
+ {
+ return new Column(Field, Data.SliceShared(offset));
Review Comment:
`Column.SliceShared(int offset)` creates a column backed by a `ChunkedArray`
created with `disposeArrays: true` (via `Data.SliceShared(offset)`), but this
overload does not set `disposeArrayData: true` on the returned `Column`. As a
result, disposing the returned shared slice won't dispose its
`ChunkedArray`/arrays and can leak retained references. It should mirror the
`(offset, length)` overload by opting into disposal of the sliced data.
```suggestion
return new Column(Field, Data.SliceShared(offset),
disposeArrayData: true);
```
##########
src/Apache.Arrow/Arrays/ArrayData.cs:
##########
@@ -149,6 +156,52 @@ public ArrayData Slice(int offset, int length)
return new ArrayData(DataType, length, nullCount, offset, Buffers,
Children, Dictionary);
}
+ /// <summary>
+ /// Slice this ArrayData with shared ownership. The returned slice
keeps the
+ /// underlying buffers alive via reference counting. The caller must
dispose the
+ /// returned ArrayData when done.
+ /// </summary>
+ public ArrayData SliceShared(int offset, int length)
+ {
+ if (offset > Length)
+ {
+ throw new ArgumentException($"Offset {offset} cannot be
greater than Length {Length} for Array.SliceShared");
+ }
+
+ length = Math.Min(Length - offset, length);
+ offset += Offset;
+
+ int nullCount;
+ if (NullCount == 0)
+ {
+ nullCount = 0;
+ }
+ else if (NullCount == Length)
+ {
+ nullCount = length;
+ }
+ else if (offset == Offset && length == Length)
+ {
+ nullCount = NullCount;
+ }
+ else
+ {
+ nullCount = RecalculateNullCount;
+ }
+
+ ArrowBuffer[] retainedBuffers = null;
+ if (Buffers != null)
+ {
+ retainedBuffers = new ArrowBuffer[Buffers.Length];
+ for (int i = 0; i < Buffers.Length; i++)
+ {
+ retainedBuffers[i] = Buffers[i].Retain();
+ }
+ }
+
+ return new ArrayData(DataType, length, nullCount, offset,
retainedBuffers, Children, Dictionary);
Review Comment:
`SliceShared` retains the top-level `Buffers`, but it reuses `Children` and
`Dictionary` without retaining/cloning them. Because `ArrayData.Dispose()`
disposes `Children`/`Dictionary`, disposing the original array can dispose
these objects out from under the shared slice (and later disposing the slice
can double-dispose them). `SliceShared` should ensure child/dictionary
`ArrayData` remain valid independently (e.g., by retaining/cloning them
similarly to buffers, potentially recursively).
```suggestion
ArrayData[] clonedChildren = null;
if (Children != null)
{
clonedChildren = new ArrayData[Children.Length];
for (int i = 0; i < Children.Length; i++)
{
clonedChildren[i] = Children[i]?.Clone();
}
}
ArrayData clonedDictionary = Dictionary?.Clone();
return new ArrayData(DataType, length, nullCount, offset,
retainedBuffers, clonedChildren, clonedDictionary);
```
##########
src/Apache.Arrow/ChunkedArray.cs:
##########
@@ -92,6 +102,55 @@ public ChunkedArray Slice(long offset)
return Slice(offset, Length - offset);
}
+ /// <summary>
+ /// Slice this chunked array with shared ownership. The returned slice
keeps the
+ /// underlying buffers alive via reference counting. The caller must
dispose
+ /// the returned chunked array when done.
+ /// </summary>
+ public ChunkedArray SliceShared(long offset, long length)
+ {
+ if (offset >= Length)
+ {
+ throw new ArgumentException($"Index {offset} cannot be greater
than the Column's Length {Length}");
Review Comment:
`ChunkedArray.SliceShared` throws an exception message that references
"Index" and "the Column's Length", which doesn't match the method/type name
(and also doesn't reflect the `offset >= Length` condition accurately).
Consider aligning this message with `ChunkedArray.Slice`/other `SliceShared`
methods (e.g., "Offset {offset} cannot be greater than or equal to Length
{Length} for ChunkedArray.SliceShared").
```suggestion
throw new ArgumentException($"Offset {offset} cannot be
greater than or equal to Length {Length} for ChunkedArray.SliceShared");
```
##########
src/Apache.Arrow/Memory/ExportedAllocationOwner.cs:
##########
@@ -49,10 +49,18 @@ public IntPtr Acquire(IntPtr ptr, int offset, int length)
public unsafe IntPtr Reference(MemoryHandle handle)
{
- _handles.Add(handle);
+ _disposables.Add(handle);
return new IntPtr(handle.Pointer);
}
Review Comment:
`ExportedAllocationOwner.Reference` stores `MemoryHandle` in a
`List<IDisposable>`, which boxes the `MemoryHandle` struct and adds per-buffer
allocations/GC pressure during export. If export performance matters, consider
keeping a `List<MemoryHandle>` for pin handles and a separate list for other
`IDisposable` instances (e.g., `SharedMemoryHandle`) to avoid boxing.
##########
test/Apache.Arrow.Tests/ArrowBufferTests.cs:
##########
@@ -15,6 +15,8 @@
using System;
using System.Runtime.InteropServices;
+using System.Threading;
+using System.Threading.Tasks;
Review Comment:
`System.Threading.Tasks` is imported but not used in this file. This
repository enables `TreatWarningsAsErrors`, so the unused using will fail the
build (CS8019). Remove the unused using or use it.
```suggestion
```
--
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]
