Support-itecz commented on code in PR #1376:
URL: https://github.com/apache/arrow-java/pull/1376#discussion_r4335318145
##########
vector/src/main/java/org/apache/arrow/vector/BaseFixedWidthVector.java:
##########
@@ -833,10 +832,12 @@ public void copyFrom(int fromIndex, int thisIndex,
ValueVector from) {
BitVectorHelper.unsetBit(this.getValidityBuffer(), thisIndex);
} else {
BitVectorHelper.setBit(this.getValidityBuffer(), thisIndex);
- MemoryUtil.copyMemory(
- from.getDataBuffer().memoryAddress() + (long) fromIndex * typeWidth,
- this.getDataBuffer().memoryAddress() + (long) thisIndex * typeWidth,
- typeWidth);
+ from.getDataBuffer()
Review Comment:
`VectorAppender` and `BitVectorHelper.concatBits` are reachable: the delta
vector can be one loaded via `loadFieldBuffers`, and nothing checks its data,
view, type or validity buffer against its value count before the raw copy. I
added a source `checkBytes` before each of those copies and both `concatBits`
inputs, plus two appender tests (short data buffer, short validity buffer) that
fail without the change. The offset copies were already bounded by the
`getInt`/`getLong` of the last offset right above them.
`DecimalVector`/`Decimal256Vector` `setSafe` and `setBigEndianSafe` already
`checkBytes` both buffers before copying, so I left those alone. The
description now lists what is covered.
##########
vector/src/main/java/org/apache/arrow/vector/BaseFixedWidthVector.java:
##########
@@ -833,10 +832,12 @@ public void copyFrom(int fromIndex, int thisIndex,
ValueVector from) {
BitVectorHelper.unsetBit(this.getValidityBuffer(), thisIndex);
} else {
BitVectorHelper.setBit(this.getValidityBuffer(), thisIndex);
- MemoryUtil.copyMemory(
- from.getDataBuffer().memoryAddress() + (long) fromIndex * typeWidth,
- this.getDataBuffer().memoryAddress() + (long) thisIndex * typeWidth,
- typeWidth);
+ from.getDataBuffer()
Review Comment:
Ran `Float8Benchmarks.copyFromBenchmark` (1024 copies per op, 2 forks x 8
iterations on a laptop, so the error bars are wide):
- checks on: main 13.98 ± 6.03 us/op, this PR 19.89 ± 5.18 us/op
- `enable_unsafe_memory_access=true`: main 8.51 ± 4.87, this PR 8.79 ± 4.83
So about 6 ns per value with checks on and nothing measurable with them off.
I also tried `checkBytes` on both buffers plus the original raw copy instead of
`getBytes`, and it came out the same (20.38 ± 11.60), so I kept `getBytes`. The
appender changes are one range check per buffer per append, not per value.
Agreed on the unsafe flag. With it set the source check is skipped and the
read is unchecked again. That is in the description now.
##########
vector/src/test/java/org/apache/arrow/vector/TestCopyFrom.java:
##########
@@ -1098,4 +1102,25 @@ public void testCopySafeArrow7837() {
assertEquals("1234567890",
Objects.requireNonNull(vc2.getObject(5)).toString());
}
}
+
+ @Test
+ public void testCopyFromFixedWidthSourceBufferTooShort() {
+ try (IntVector source = new IntVector("source", allocator);
+ IntVector target = new IntVector("target", allocator);
+ ArrowBuf body = allocator.buffer(64)) {
+ body.setZero(0, body.capacity());
+ body.setInt(0, 1);
+ body.setInt(12, 0xBAD);
+ // Field node claims 4 values, but the data buffer only holds one, as a
malformed IPC
Review Comment:
Good catch. Today it works because `loadValidityBuffer` replaces an empty
validity buffer with an all-ones one when the null count is 0, not because of
`isSet`, but the test should not rely on that. The source now gets a real
8-byte validity buffer with all 4 bits set, and the new appender tests do the
same.
--
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]
