Skip to content

Commit 67cd552

Browse files
andygroveandygrove
committed
refactor(vector): tidy decimal length validation and tests
Use Preconditions.checkArgument with TYPE_WIDTH in the message, replace a stray 32 literal, collapse the oversized-write tests into one helper, add a zero-length test for setBigEndianSafe(ArrowBuf), and shorten comments so they fit the 100 column limit enforced by spotless.
1 parent 5892f8f commit 67cd552

4 files changed

Lines changed: 84 additions & 96 deletions

File tree

‎vector/src/main/java/org/apache/arrow/vector/Decimal256Vector.java‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import org.apache.arrow.memory.ArrowBuf;
2424
import org.apache.arrow.memory.BufferAllocator;
2525
import org.apache.arrow.memory.util.MemoryUtil;
26+
import org.apache.arrow.util.Preconditions;
2627
import org.apache.arrow.vector.complex.impl.Decimal256ReaderImpl;
2728
import org.apache.arrow.vector.complex.reader.FieldReader;
2829
import org.apache.arrow.vector.holders.Decimal256Holder;
@@ -237,16 +238,12 @@ public void setBigEndian(int index, byte[] value) {
237238
}
238239
}
239240

240-
/**
241-
* Validates the byte length of a raw decimal value before it is written. The raw write paths
242-
* copy straight into native memory and bypass {@link ArrowBuf} bounds checking, so a length
243-
* larger than {@link #TYPE_WIDTH} would overrun the value slot.
244-
*/
245241
private static void checkValueLength(int length) {
246-
if (length < 0 || length > TYPE_WIDTH) {
247-
throw new IllegalArgumentException(
248-
"Invalid decimal value length. Valid length in [1 - 32], got " + length);
249-
}
242+
Preconditions.checkArgument(
243+
length >= 0 && length <= TYPE_WIDTH,
244+
"Invalid decimal value length. Valid length in [0 - %s], got %s",
245+
TYPE_WIDTH,
246+
length);
250247
}
251248

252249
/**
@@ -334,7 +331,7 @@ public void setBigEndianSafe(int index, long start, ArrowBuf buffer, int length)
334331
MemoryUtil.putByte(outAddress + byteIdx, val);
335332
}
336333
// sign extend
337-
if (length < 32) {
334+
if (length < TYPE_WIDTH) {
338335
byte msb = MemoryUtil.getByte(inAddress);
339336
final byte pad = (byte) (msb < 0 ? 0xFF : 0x00);
340337
MemoryUtil.setMemory(outAddress + length, Decimal256Vector.TYPE_WIDTH - length, pad);

‎vector/src/main/java/org/apache/arrow/vector/DecimalVector.java‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import org.apache.arrow.memory.ArrowBuf;
2424
import org.apache.arrow.memory.BufferAllocator;
2525
import org.apache.arrow.memory.util.MemoryUtil;
26+
import org.apache.arrow.util.Preconditions;
2627
import org.apache.arrow.vector.complex.impl.DecimalReaderImpl;
2728
import org.apache.arrow.vector.complex.reader.FieldReader;
2829
import org.apache.arrow.vector.holders.DecimalHolder;
@@ -236,16 +237,12 @@ public void setBigEndian(int index, byte[] value) {
236237
}
237238
}
238239

239-
/**
240-
* Validates the byte length of a raw decimal value before it is written. The raw write paths
241-
* copy straight into native memory and bypass {@link ArrowBuf} bounds checking, so a length
242-
* larger than {@link #TYPE_WIDTH} would overrun the value slot.
243-
*/
244240
private static void checkValueLength(int length) {
245-
if (length < 0 || length > TYPE_WIDTH) {
246-
throw new IllegalArgumentException(
247-
"Invalid decimal value length. Valid length in [1 - 16], got " + length);
248-
}
241+
Preconditions.checkArgument(
242+
length >= 0 && length <= TYPE_WIDTH,
243+
"Invalid decimal value length. Valid length in [0 - %s], got %s",
244+
TYPE_WIDTH,
245+
length);
249246
}
250247

251248
/**

‎vector/src/test/java/org/apache/arrow/vector/TestDecimal256Vector.java‎

Lines changed: 34 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
import java.math.BigDecimal;
2626
import java.math.BigInteger;
27+
import java.util.function.BiConsumer;
2728
import org.apache.arrow.memory.ArrowBuf;
2829
import org.apache.arrow.memory.BufferAllocator;
2930
import org.apache.arrow.vector.types.pojo.ArrowType;
@@ -269,7 +270,7 @@ public void decimalBE2NE() {
269270
IllegalArgumentException.class,
270271
() -> decimalVector.setBigEndian(insertionIdxCapture, new byte[33]));
271272
assertTrue(
272-
ex.getMessage().equals("Invalid decimal value length. Valid length in [1 - 32], got 33"));
273+
ex.getMessage().equals("Invalid decimal value length. Valid length in [0 - 32], got 33"));
273274
decimalVector.setValueCount(insertionIdx);
274275

275276
// retrieve values and check if they are correct
@@ -421,13 +422,6 @@ private void verifyWritingArrowBufWithBigEndianBytes(
421422
}
422423
}
423424

424-
// ---- Oversized raw writes -------------------------------------------------------------------
425-
// The value buffer and validity buffer are adjacent slices of one allocation (value buffer first),
426-
// so a write that overruns the last value slot lands in the validity buffer. That makes an overrun
427-
// observable from Java via isNull() without touching memory outside the allocation. Each test
428-
// marks the first 8 values valid, attempts an oversized write into the last slot whose spilled
429-
// byte is zero, and asserts the write is rejected without clearing any validity bit.
430-
431425
private static final int OVERSIZED_LENGTH = Decimal256Vector.TYPE_WIDTH + 1;
432426

433427
private Decimal256Vector newVectorWithFirstEightValid() {
@@ -436,51 +430,44 @@ private Decimal256Vector newVectorWithFirstEightValid() {
436430
for (int i = 0; i < 8; i++) {
437431
vector.setSafe(i, BigDecimal.valueOf(i));
438432
}
439-
vector.setValueCount(8);
440433
return vector;
441434
}
442435

436+
// Validity bits sit right after the value buffer, so an overrun of the last slot clobbers them.
443437
private static void assertFirstEightValid(Decimal256Vector vector) {
444438
for (int i = 0; i < 8; i++) {
445439
assertFalse(vector.isNull(i), "validity bit " + i + " was overwritten by an oversized write");
446440
assertEquals(BigDecimal.valueOf(i), vector.getObject(i));
447441
}
448442
}
449443

450-
@Test
451-
public void setBigEndianSafeRejectsOversizedByteArrayWithoutWriting() {
452-
try (Decimal256Vector vector = newVectorWithFirstEightValid()) {
453-
final int last = vector.getValueCapacity() - 1;
454-
final byte[] oversized = new byte[OVERSIZED_LENGTH];
455-
assertThrows(
456-
IllegalArgumentException.class, () -> vector.setBigEndianSafe(last, oversized));
457-
assertFirstEightValid(vector);
458-
}
459-
}
460-
461-
@Test
462-
public void setSafeFromArrowBufRejectsOversizedLengthWithoutWriting() {
444+
private void assertOversizedWriteRejected(BiConsumer<Decimal256Vector, ArrowBuf> write) {
463445
try (Decimal256Vector vector = newVectorWithFirstEightValid();
464446
ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) {
465447
src.setZero(0, OVERSIZED_LENGTH);
466-
final int last = vector.getValueCapacity() - 1;
467-
assertThrows(
468-
IllegalArgumentException.class, () -> vector.setSafe(last, 0, src, OVERSIZED_LENGTH));
448+
assertThrows(IllegalArgumentException.class, () -> write.accept(vector, src));
469449
assertFirstEightValid(vector);
470450
}
471451
}
472452

473453
@Test
474-
public void setBigEndianSafeFromArrowBufRejectsOversizedLengthWithoutWriting() {
475-
try (Decimal256Vector vector = newVectorWithFirstEightValid();
476-
ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) {
477-
src.setZero(0, OVERSIZED_LENGTH);
478-
final int last = vector.getValueCapacity() - 1;
479-
assertThrows(
480-
IllegalArgumentException.class,
481-
() -> vector.setBigEndianSafe(last, 0, src, OVERSIZED_LENGTH));
482-
assertFirstEightValid(vector);
483-
}
454+
public void setBigEndianSafeRejectsOversizedByteArray() {
455+
assertOversizedWriteRejected(
456+
(vector, src) ->
457+
vector.setBigEndianSafe(vector.getValueCapacity() - 1, new byte[OVERSIZED_LENGTH]));
458+
}
459+
460+
@Test
461+
public void setSafeFromArrowBufRejectsOversizedLength() {
462+
assertOversizedWriteRejected(
463+
(vector, src) -> vector.setSafe(vector.getValueCapacity() - 1, 0, src, OVERSIZED_LENGTH));
464+
}
465+
466+
@Test
467+
public void setBigEndianSafeFromArrowBufRejectsOversizedLength() {
468+
assertOversizedWriteRejected(
469+
(vector, src) ->
470+
vector.setBigEndianSafe(vector.getValueCapacity() - 1, 0, src, OVERSIZED_LENGTH));
484471
}
485472

486473
@Test
@@ -492,7 +479,18 @@ public void setSafeFromArrowBufWithZeroLengthDoesNotReadBeforeStart() {
492479
src.setByte(0, (byte) 0xFF);
493480
src.setByte(1, (byte) 0x00);
494481
vector.setSafe(0, 1, src, 0);
495-
vector.setValueCount(1);
482+
assertEquals(BigDecimal.ZERO, vector.getObject(0));
483+
}
484+
}
485+
486+
@Test
487+
public void setBigEndianSafeFromArrowBufWithZeroLengthDoesNotReadAtStart() {
488+
try (Decimal256Vector vector = new Decimal256Vector("decimal", allocator, 38, 0);
489+
ArrowBuf src = allocator.buffer(1)) {
490+
vector.allocateNew(1);
491+
// A negative byte at `start` must not be used for sign extension of an empty value.
492+
src.setByte(0, (byte) 0xFF);
493+
vector.setBigEndianSafe(0, 0, src, 0);
496494
assertEquals(BigDecimal.ZERO, vector.getObject(0));
497495
}
498496
}

‎vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java‎

Lines changed: 37 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,15 @@
1717
package org.apache.arrow.vector;
1818

1919
import static org.junit.jupiter.api.Assertions.assertEquals;
20-
import static org.junit.jupiter.api.Assertions.assertThrows;
2120
import static org.junit.jupiter.api.Assertions.assertFalse;
2221
import static org.junit.jupiter.api.Assertions.assertSame;
22+
import static org.junit.jupiter.api.Assertions.assertThrows;
2323
import static org.junit.jupiter.api.Assertions.assertTrue;
2424
import static org.junit.jupiter.api.Assertions.fail;
2525

2626
import java.math.BigDecimal;
2727
import java.math.BigInteger;
28+
import java.util.function.BiConsumer;
2829
import org.apache.arrow.memory.ArrowBuf;
2930
import org.apache.arrow.memory.BufferAllocator;
3031
import org.apache.arrow.vector.types.pojo.ArrowType;
@@ -265,7 +266,7 @@ public void decimalBE2NE() {
265266
} catch (IllegalArgumentException ex) {
266267
assertTrue(
267268
ex.getMessage()
268-
.equals("Invalid decimal value length. Valid length in [1 - 16], got 17"));
269+
.equals("Invalid decimal value length. Valid length in [0 - 16], got 17"));
269270
}
270271
decimalVector.setValueCount(insertionIdx);
271272

@@ -416,80 +417,75 @@ private void verifyWritingArrowBufWithBigEndianBytes(
416417
}
417418
}
418419

419-
// ---- Oversized raw writes -------------------------------------------------------------------
420-
// The value buffer and validity buffer are adjacent slices of one allocation (value buffer first),
421-
// so a write that overruns the last value slot lands in the validity buffer. That makes an overrun
422-
// observable from Java via isNull() without touching memory outside the allocation. Each test
423-
// marks the first 8 values valid, attempts an oversized write into the last slot whose spilled
424-
// byte is zero, and asserts the write is rejected without clearing any validity bit.
425-
426420
private static final int OVERSIZED_LENGTH = DecimalVector.TYPE_WIDTH + 1;
427421

428422
private DecimalVector newVectorWithFirstEightValid() {
429-
DecimalVector vector = TestUtils.newVector(
430-
DecimalVector.class, "decimal", new ArrowType.Decimal(38, 0, 128), allocator);
423+
DecimalVector vector = new DecimalVector("decimal", allocator, 38, 0);
431424
vector.allocateNew(64);
432425
for (int i = 0; i < 8; i++) {
433426
vector.setSafe(i, BigDecimal.valueOf(i));
434427
}
435-
vector.setValueCount(8);
436428
return vector;
437429
}
438430

431+
// Validity bits sit right after the value buffer, so an overrun of the last slot clobbers them.
439432
private static void assertFirstEightValid(DecimalVector vector) {
440433
for (int i = 0; i < 8; i++) {
441434
assertFalse(vector.isNull(i), "validity bit " + i + " was overwritten by an oversized write");
442435
assertEquals(BigDecimal.valueOf(i), vector.getObject(i));
443436
}
444437
}
445438

446-
@Test
447-
public void setBigEndianSafeRejectsOversizedByteArrayWithoutWriting() {
448-
try (DecimalVector vector = newVectorWithFirstEightValid()) {
449-
final int last = vector.getValueCapacity() - 1;
450-
final byte[] oversized = new byte[OVERSIZED_LENGTH];
451-
assertThrows(
452-
IllegalArgumentException.class, () -> vector.setBigEndianSafe(last, oversized));
453-
assertFirstEightValid(vector);
454-
}
455-
}
456-
457-
@Test
458-
public void setSafeFromArrowBufRejectsOversizedLengthWithoutWriting() {
439+
private void assertOversizedWriteRejected(BiConsumer<DecimalVector, ArrowBuf> write) {
459440
try (DecimalVector vector = newVectorWithFirstEightValid();
460441
ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) {
461442
src.setZero(0, OVERSIZED_LENGTH);
462-
final int last = vector.getValueCapacity() - 1;
463-
assertThrows(
464-
IllegalArgumentException.class, () -> vector.setSafe(last, 0, src, OVERSIZED_LENGTH));
443+
assertThrows(IllegalArgumentException.class, () -> write.accept(vector, src));
465444
assertFirstEightValid(vector);
466445
}
467446
}
468447

469448
@Test
470-
public void setBigEndianSafeFromArrowBufRejectsOversizedLengthWithoutWriting() {
471-
try (DecimalVector vector = newVectorWithFirstEightValid();
472-
ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) {
473-
src.setZero(0, OVERSIZED_LENGTH);
474-
final int last = vector.getValueCapacity() - 1;
475-
assertThrows(
476-
IllegalArgumentException.class,
477-
() -> vector.setBigEndianSafe(last, 0, src, OVERSIZED_LENGTH));
478-
assertFirstEightValid(vector);
479-
}
449+
public void setBigEndianSafeRejectsOversizedByteArray() {
450+
assertOversizedWriteRejected(
451+
(vector, src) ->
452+
vector.setBigEndianSafe(vector.getValueCapacity() - 1, new byte[OVERSIZED_LENGTH]));
453+
}
454+
455+
@Test
456+
public void setSafeFromArrowBufRejectsOversizedLength() {
457+
assertOversizedWriteRejected(
458+
(vector, src) -> vector.setSafe(vector.getValueCapacity() - 1, 0, src, OVERSIZED_LENGTH));
459+
}
460+
461+
@Test
462+
public void setBigEndianSafeFromArrowBufRejectsOversizedLength() {
463+
assertOversizedWriteRejected(
464+
(vector, src) ->
465+
vector.setBigEndianSafe(vector.getValueCapacity() - 1, 0, src, OVERSIZED_LENGTH));
480466
}
481467

482468
@Test
483469
public void setSafeFromArrowBufWithZeroLengthDoesNotReadBeforeStart() {
484-
try (DecimalVector vector = TestUtils.newVector(
485-
DecimalVector.class, "decimal", new ArrowType.Decimal(38, 0, 128), allocator);
470+
try (DecimalVector vector = new DecimalVector("decimal", allocator, 38, 0);
486471
ArrowBuf src = allocator.buffer(2)) {
487472
vector.allocateNew(1);
488473
// A negative byte immediately before `start` must not be used for sign extension.
489474
src.setByte(0, (byte) 0xFF);
490475
src.setByte(1, (byte) 0x00);
491476
vector.setSafe(0, 1, src, 0);
492-
vector.setValueCount(1);
477+
assertEquals(BigDecimal.ZERO, vector.getObject(0));
478+
}
479+
}
480+
481+
@Test
482+
public void setBigEndianSafeFromArrowBufWithZeroLengthDoesNotReadAtStart() {
483+
try (DecimalVector vector = new DecimalVector("decimal", allocator, 38, 0);
484+
ArrowBuf src = allocator.buffer(1)) {
485+
vector.allocateNew(1);
486+
// A negative byte at `start` must not be used for sign extension of an empty value.
487+
src.setByte(0, (byte) 0xFF);
488+
vector.setBigEndianSafe(0, 0, src, 0);
493489
assertEquals(BigDecimal.ZERO, vector.getObject(0));
494490
}
495491
}

0 commit comments

Comments
 (0)