mbutrovich commented on code in PR #1192:
URL: https://github.com/apache/arrow-java/pull/1192#discussion_r3546956150
##########
vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java:
##########
@@ -1405,7 +1405,8 @@ public final int getTotalValueLengthUpToIndex(int index) {
}
protected final void handleSafe(int index, int dataLength) {
- final long targetCapacity = roundUpToMultipleOf16((long) index *
ELEMENT_SIZE + dataLength);
+ // The view buffer stores one fixed-width view per value; payload bytes
are allocated separately.
+ final long targetCapacity = roundUpToMultipleOf16(((long) index + 1) *
ELEMENT_SIZE);
Review Comment:
Correct. Reserves the full slot regardless of payload; `((long) index + 1)`
avoids overflow. `roundUpToMultipleOf16` is a no-op here but fine to keep.
##########
vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java:
##########
@@ -540,6 +540,26 @@ public void testSizeOfViewBufferElements() {
}
}
+ @ParameterizedTest
+ @MethodSource({"vectorCreatorProvider"})
+ public void testSetSafeEmptyValueAtViewBufferBoundary(
Review Comment:
Only covers the empty-value path. `setNull` and
`fillEmpties`/`setValueCount` reach the same `handleSafe(index, 0)`. Suggest
adding tests for those.
## Suggested tests (imports already present)
### setValueCount trailing gap (silent trigger, no empty string written)
```java
@ParameterizedTest
@MethodSource({"vectorCreatorProvider"})
public void testSetValueCountFillsEmptiesAtViewBufferBoundary(
Function<BufferAllocator, BaseVariableWidthViewVector> vectorCreator) {
try (final BaseVariableWidthViewVector vector =
vectorCreator.apply(allocator)) {
vector.allocateNew();
final int valueCapacity = vector.getValueCapacity();
vector.setSafe(valueCapacity - 1, "x".getBytes(StandardCharsets.UTF_8));
// Leaves index valueCapacity unset: fillEmpties -> handleSafe(_, 0) on
a full buffer.
vector.setValueCount(valueCapacity + 1);
assertTrue(vector.getValueCapacity() > valueCapacity);
assertTrue(vector.isNull(valueCapacity));
}
}
```
### setNull at the boundary
```java
@ParameterizedTest
@MethodSource({"vectorCreatorProvider"})
public void testSetNullAtViewBufferBoundary(
Function<BufferAllocator, BaseVariableWidthViewVector> vectorCreator) {
try (final BaseVariableWidthViewVector vector =
vectorCreator.apply(allocator)) {
vector.allocateNew();
final int valueCapacity = vector.getValueCapacity();
for (int i = 0; i <= valueCapacity; i++) {
vector.setNull(i);
}
vector.setValueCount(valueCapacity + 1);
assertTrue(vector.getValueCapacity() > valueCapacity);
assertTrue(vector.isNull(valueCapacity));
}
}
```
##########
vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java:
##########
@@ -540,6 +540,26 @@ public void testSizeOfViewBufferElements() {
}
}
+ @ParameterizedTest
+ @MethodSource({"vectorCreatorProvider"})
+ public void testSetSafeEmptyValueAtViewBufferBoundary(
Review Comment:
Only covers the empty-value path. `setNull` and
`fillEmpties`/`setValueCount` reach the same `handleSafe(index, 0)`. Suggest
adding tests for those.
### setValueCount trailing gap (silent trigger, no empty string written)
```java
@ParameterizedTest
@MethodSource({"vectorCreatorProvider"})
public void testSetValueCountFillsEmptiesAtViewBufferBoundary(
Function<BufferAllocator, BaseVariableWidthViewVector> vectorCreator) {
try (final BaseVariableWidthViewVector vector =
vectorCreator.apply(allocator)) {
vector.allocateNew();
final int valueCapacity = vector.getValueCapacity();
vector.setSafe(valueCapacity - 1, "x".getBytes(StandardCharsets.UTF_8));
// Leaves index valueCapacity unset: fillEmpties -> handleSafe(_, 0) on
a full buffer.
vector.setValueCount(valueCapacity + 1);
assertTrue(vector.getValueCapacity() > valueCapacity);
assertTrue(vector.isNull(valueCapacity));
}
}
```
### setNull at the boundary
```java
@ParameterizedTest
@MethodSource({"vectorCreatorProvider"})
public void testSetNullAtViewBufferBoundary(
Function<BufferAllocator, BaseVariableWidthViewVector> vectorCreator) {
try (final BaseVariableWidthViewVector vector =
vectorCreator.apply(allocator)) {
vector.allocateNew();
final int valueCapacity = vector.getValueCapacity();
for (int i = 0; i <= valueCapacity; i++) {
vector.setNull(i);
}
vector.setValueCount(valueCapacity + 1);
assertTrue(vector.getValueCapacity() > valueCapacity);
assertTrue(vector.isNull(valueCapacity));
}
}
```
--
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]
