jbonofre commented on code in PR #1376:
URL: https://github.com/apache/arrow-java/pull/1376#discussion_r4480640069
##########
vector/src/main/java/org/apache/arrow/vector/util/VectorAppender.java:
##########
@@ -366,6 +369,10 @@ public ValueVector visit(BaseVariableWidthViewVector
deltaVector, Void value) {
// append view buffer
ArrowBuf targetViewBuffer = targetVector.getDataBuffer();
+ deltaVector
+ .getDataBuffer()
+ .checkBytes(
+ 0, (long) BaseVariableWidthViewVector.ELEMENT_SIZE *
deltaVector.getValueCount());
Review Comment:
By the time this check runs, the delta variadic data buffers have already
been retained and added to the target `dataBuffers`, and the validity bits have
been appended. If it throws, the target keeps those buffers: they stay pinned
until the target is closed, they are written out as extra variadic buffers on
the next IPC write of the target, and every retry adds more.
Can we move this check (plus a validity check) above the capacity loop,
before the `retain()`/`addAll()`, using `checkCapacity` so a rejected delta
leaves the target untouched:
```java
checkCapacity(
deltaVector.getDataBuffer(),
(long) BaseVariableWidthViewVector.ELEMENT_SIZE *
deltaVector.getValueCount(),
"view");
```
##########
vector/src/main/java/org/apache/arrow/vector/util/VectorAppender.java:
##########
@@ -99,6 +99,9 @@ public ValueVector visit(BaseFixedWidthVector deltaVector,
Void value) {
deltaVector.getValueCount(),
targetVector.getDataBuffer());
} else {
+ deltaVector
+ .getDataBuffer()
+ .checkBytes(0, (long) deltaVector.getTypeWidth() *
deltaVector.getValueCount());
Review Comment:
This check runs after the target has already been reallocated to
`newValueCount` and after `concatBits` has copied the delta validity bits into
the target. So a malformed delta (e.g. an IPC delta dictionary whose field node
claims ~2^30 values over a 4-byte data buffer) can force a multi GB
reallocation of the target before it is rejected, and if that allocation
succeeds the target is left with stray validity bits past its value count.
#1364 uses the opposite order for the variable width paths: `validateDelta`
runs before the target is touched, so a rejected delta leaves the target
unchanged.
Could we do the same here, right after the `getValueCount() == 0` early
return? Something like:
```java
int deltaCount = deltaVector.getValueCount();
checkCapacity(
deltaVector.getValidityBuffer(),
BitVectorHelper.getValidityBufferSizeFromCount(deltaCount),
"validity");
long dataSize =
deltaVector instanceof BitVector
? BitVectorHelper.getValidityBufferSizeFromCount(deltaCount)
: (long) deltaVector.getTypeWidth() * deltaCount;
checkCapacity(deltaVector.getDataBuffer(), dataSize, "data");
```
The union (line 682) and dense union (line 748) type buffer checks have the
same problem: they run after `reAlloc()`. They should move above the capacity
loop too, e.g. `checkCapacity(deltaVector.getTypeBuffer(), (long)
deltaVector.getValueCount() * UnionVector.TYPE_WIDTH, "type")`.
--
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]
