adamreeve commented on code in PR #291:
URL: https://github.com/apache/arrow-dotnet/pull/291#discussion_r2991763697
##########
src/Apache.Arrow/Arrays/ArrayData.cs:
##########
@@ -97,8 +101,61 @@ public ArrayData(
Dictionary = dictionary;
}
+ private ArrayData(
+ ArrayData parent,
+ IArrowType dataType,
+ int length, int nullCount, int offset,
+ ArrowBuffer[] buffers, ArrayData[] children, ArrayData dictionary)
+ {
+ _parent = parent;
+ DataType = dataType ?? NullType.Default;
+ Length = length;
+ NullCount = nullCount;
+ Offset = offset;
+ Buffers = buffers;
+ Children = children;
+ Dictionary = dictionary;
+ }
+
+ /// <summary>
+ /// Increment the reference count, allowing this ArrayData to be shared
+ /// across multiple owners. Each call to Acquire must be balanced by a
+ /// call to <see cref="Dispose"/>.
+ /// </summary>
+ /// <returns>This instance.</returns>
+ public ArrayData Acquire()
+ {
+ if (Interlocked.Increment(ref _referenceCount) <= 1)
+ {
+ Interlocked.Decrement(ref _referenceCount);
+ throw new ObjectDisposedException(nameof(ArrayData));
+ }
+ return this;
+ }
+
public void Dispose()
{
+ int remaining = Interlocked.Decrement(ref _referenceCount);
+ if (remaining > 0)
+ {
+ return;
+ }
+ if (remaining < 0)
+ {
+ // Dispose is idempotent — multiple dispose calls are safe.
+ // This can occur when e.g. ListArray disposes its Values (a
child)
+ // and then the parent ArrayData also disposes the same child.
Review Comment:
I don't quite follow this. If the `ListArray` is a slice, then it won't
dispose the values child because they're owned by the parent, according to the
code below.
##########
src/Apache.Arrow/ChunkedArray.cs:
##########
@@ -64,12 +64,12 @@ public ChunkedArray Slice(long offset, long length)
{
if (offset >= Length)
{
- throw new ArgumentException($"Index {offset} cannot be greater
than the Column's Length {Length}");
+ throw new ArgumentException($"Offset {offset} cannot be
greater than Length {Length} for ChunkedArray.Slice");
}
int curArrayIndex = 0;
int numArrays = Arrays.Count;
- while (curArrayIndex < numArrays && offset >
Arrays[curArrayIndex].Length)
+ while (curArrayIndex < numArrays && offset >=
Arrays[curArrayIndex].Length)
Review Comment:
😬
##########
src/Apache.Arrow/Arrays/ArrayData.cs:
##########
@@ -97,8 +101,61 @@ public ArrayData(
Dictionary = dictionary;
}
+ private ArrayData(
+ ArrayData parent,
+ IArrowType dataType,
+ int length, int nullCount, int offset,
+ ArrowBuffer[] buffers, ArrayData[] children, ArrayData dictionary)
+ {
+ _parent = parent;
+ DataType = dataType ?? NullType.Default;
+ Length = length;
+ NullCount = nullCount;
+ Offset = offset;
+ Buffers = buffers;
+ Children = children;
+ Dictionary = dictionary;
+ }
+
+ /// <summary>
+ /// Increment the reference count, allowing this ArrayData to be shared
+ /// across multiple owners. Each call to Acquire must be balanced by a
+ /// call to <see cref="Dispose"/>.
+ /// </summary>
+ /// <returns>This instance.</returns>
+ public ArrayData Acquire()
+ {
+ if (Interlocked.Increment(ref _referenceCount) <= 1)
+ {
+ Interlocked.Decrement(ref _referenceCount);
+ throw new ObjectDisposedException(nameof(ArrayData));
+ }
+ return this;
Review Comment:
I think two concurrent `Acquire` calls on a disposed object could cause this
method to incorrectly return `this` rather than throw. Eg:
```
_referenceCount = 0
thread_0 increments, gets 1
thread_1 increments, gets 2, and returns this.
thread_0 decrements and _referenceCount is now 1. The ArrayData has been
brought back to life.
```
Claude came up with this alternative approach which looks safer to me:
```c#
int currentRefCount;
do
{
currentRefCount = Volatile.Read(ref _referenceCount);
if (currentRefCount <= 0)
{
throw new ObjectDisposedException(nameof(ArrayData));
}
} while (Interlocked.CompareExchange(ref _referenceCount, currentRefCount +
1, currentRefCount) != currentRefCount);
return 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
