diff --git a/CHANGELOG.md b/CHANGELOG.md index 820a1989d..77a29fadb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Breaking:** `ExtensionId` is a sealed `WellKnown`/`Custom` type like `EncodingId`, so third-party extension ids can be named, and `ExtensionId.parse` returns the id instead of an `Optional`. ([#530](https://github.com/dfa1/vortex-java/issues/530)) ### Fixed +- Frame-of-reference encoding takes the unsigned minimum of a `u64` column whose values span 2^63 as its reference, as Rust does, instead of a reference that left those values at full width. ([#571](https://github.com/dfa1/vortex-java/pull/571)) +- **Breaking:** `DType.Decimal` refuses a precision outside 1 to 76 or a scale above the precision, as Rust does, so the writer no longer writes decimal columns that Rust and vortex-java cannot read, and a variant's decimal is checked as it is read. ([#571](https://github.com/dfa1/vortex-java/pull/571)) - SQL `ORDER BY ... LIMIT` and `GROUP BY` with a few thousand result cells no longer fail with "Code grows beyond 64 KB"; past the limit Calcite scans instead. ([#569](https://github.com/dfa1/vortex-java/pull/569)) - A file whose array FlatBuffer holds a corrupt offset fails the read with `VortexException` instead of `IndexOutOfBoundsException`. ([#493](https://github.com/dfa1/vortex-java/issues/493)) - A column whose dtype does not fit its bit-packed, delta or RLE encoding fails with `VortexException` instead of `ClassCastException`, and an empty RLE boolean chunk reads. ([#493](https://github.com/dfa1/vortex-java/issues/493)) diff --git a/core/src/main/java/io/github/dfa1/vortex/core/model/DType.java b/core/src/main/java/io/github/dfa1/vortex/core/model/DType.java index fe0e1d588..1a36a44a1 100644 --- a/core/src/main/java/io/github/dfa1/vortex/core/model/DType.java +++ b/core/src/main/java/io/github/dfa1/vortex/core/model/DType.java @@ -139,7 +139,7 @@ default DType withNullable(boolean nullable) { /// @param scale number of digits to the right of the decimal point /// @return non-nullable [Decimal] static Decimal decimal(int precision, int scale) { - return new Decimal((byte) precision, (byte) scale, false); + return Decimal.of(precision, scale, false); } /// Returns a fresh [StructBuilder] for assembling a [Struct] dtype with @@ -227,6 +227,43 @@ record Primitive(PType ptype, boolean nullable) implements DType { /// @param scale number of digits to the right of the decimal point /// @param nullable whether null values are permitted record Decimal(byte precision, byte scale, boolean nullable) implements DType { + + /// Rust's `DecimalDType::try_new` bounds. Runs both on programmatic construction, so the + /// writer never emits a dtype Rust refuses, and while parsing an untrusted file's DType + /// blob, so it raises [VortexException] rather than [IllegalArgumentException] — same + /// reasoning as [Map]. + /// + /// @throws VortexException if `precision` is outside `1..76` or `scale` exceeds it + public Decimal { + requireValid(precision, scale); + } + + /// Creates a decimal from wide values, rejecting rather than truncating one that does not + /// fit the record's `byte` components (Rust's `try_into` on the proto's `u32`/`i32`). + /// + /// @param precision total number of significant decimal digits, `1..76` + /// @param scale number of digits to the right of the decimal point, at most `precision` + /// @param nullable whether null values are permitted + /// @return the decimal dtype + /// @throws VortexException if `precision` is outside `1..76`, `scale` exceeds it or does + /// not fit in a byte + public static Decimal of(int precision, int scale, boolean nullable) { + requireValid(precision, scale); + if (scale < Byte.MIN_VALUE) { + throw new VortexException("decimal scale " + scale + " does not fit in i8"); + } + return new Decimal((byte) precision, (byte) scale, nullable); + } + + // A negative scale (e.g. -2 for hundreds) is legal. + private static void requireValid(int precision, int scale) { + if (precision < 1 || precision > 76) { + throw new VortexException("decimal precision " + precision + " out of range (expected 1..76)"); + } + if (scale > precision) { + throw new VortexException("decimal scale " + scale + " exceeds precision " + precision); + } + } } /// Variable-length UTF-8 string logical type. diff --git a/core/src/test/java/io/github/dfa1/vortex/core/model/DTypeDecimalTest.java b/core/src/test/java/io/github/dfa1/vortex/core/model/DTypeDecimalTest.java new file mode 100644 index 000000000..81a64ac79 --- /dev/null +++ b/core/src/test/java/io/github/dfa1/vortex/core/model/DTypeDecimalTest.java @@ -0,0 +1,60 @@ +package io.github.dfa1.vortex.core.model; + +import io.github.dfa1.vortex.core.error.VortexException; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/// Rust's `DecimalDType::try_new` bounds. Before they lived on the record, the writer accepted +/// `DType.decimal(80, 0)` or `(10, 12)` and wrote a file that vortex-jni and our own reader refuse. +class DTypeDecimalTest { + + @ParameterizedTest + @CsvSource({"1, 0", "76, 76", "10, -5", "5, -100", "38, 0"}) + void inRange_isAccepted(int precision, int scale) { + // Given / When + DType.Decimal result = DType.Decimal.of(precision, scale, true); + + // Then + assertThat(result.precision()).isEqualTo((byte) precision); + assertThat(result.scale()).isEqualTo((byte) scale); + assertThat(result.nullable()).isTrue(); + } + + @ParameterizedTest + @CsvSource({ + "0, 0", // Rust's NonZero precision + "77, 0", // above i256's 76 digits + "300, 2", // must not truncate to (byte) 44, a valid precision + "-1, 0", // a proto u32 above 2^31 reads as a negative int + "10, 12", // scale above precision + "10, -129", // must not truncate to (byte) 127 + }) + void outOfRange_throwsVortexException(int precision, int scale) { + // Given / When / Then: VortexException, since the file parsers construct through here + assertThatThrownBy(() -> DType.Decimal.of(precision, scale, false)) + .isInstanceOf(VortexException.class); + } + + @Test + void constructor_enforcesTheSameBounds() { + // Given a byte precision of 200 — negative as a byte, and never valid + byte precision = (byte) 200; + + // When / Then + assertThatThrownBy(() -> new DType.Decimal(precision, (byte) 0, false)) + .isInstanceOf(VortexException.class) + .hasMessageContaining("out of range"); + } + + @Test + void factory_enforcesTheSameBounds() { + // Given / When / Then: the public decimal(int, int) shortcut the writer's users call + assertThatThrownBy(() -> DType.decimal(10, 12)) + .isInstanceOf(VortexException.class) + .hasMessageContaining("exceeds precision 10"); + } +} diff --git a/integration/src/test/java/io/github/dfa1/vortex/integration/WriteReadFuzzIntegrationTest.java b/integration/src/test/java/io/github/dfa1/vortex/integration/WriteReadFuzzIntegrationTest.java index 8cb2a59ce..746f49aa8 100644 --- a/integration/src/test/java/io/github/dfa1/vortex/integration/WriteReadFuzzIntegrationTest.java +++ b/integration/src/test/java/io/github/dfa1/vortex/integration/WriteReadFuzzIntegrationTest.java @@ -3,10 +3,13 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import java.io.IOException; +import java.math.BigDecimal; +import java.nio.ByteBuffer; import java.nio.channels.FileChannel; import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.util.ArrayList; +import java.util.HexFormat; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -30,6 +33,7 @@ import io.github.dfa1.vortex.reader.array.Array; import io.github.dfa1.vortex.reader.array.BoolArray; import io.github.dfa1.vortex.reader.array.ByteArray; +import io.github.dfa1.vortex.reader.array.DecimalArray; import io.github.dfa1.vortex.reader.array.DoubleArray; import io.github.dfa1.vortex.reader.array.FloatArray; import io.github.dfa1.vortex.reader.array.IntArray; @@ -67,13 +71,17 @@ @Tag("fuzz") class WriteReadFuzzIntegrationTest { + private static final HexFormat HEX = HexFormat.of(); + static { NativeLoader.loadJni(); } private static final List TYPES = List.of( DType.I8, DType.I16, DType.I32, DType.I64, DType.U8, DType.U16, DType.U32, DType.U64, - DType.F32, DType.F64, DType.BOOL, DType.UTF8); + DType.F32, DType.F64, DType.BOOL, DType.UTF8, DType.BINARY, + // One per decimal physical width Rust picks: i32, i64, i128 + DType.decimal(9, 2), DType.decimal(18, 4), DType.decimal(38, 0)); @TempDir Path tempDir; @@ -169,6 +177,10 @@ private static Object canonical(DType dtype, long raw) { return switch (dtype) { case DType.Bool _ -> (raw & 1) != 0; case DType.Utf8 _ -> raw % 97 == 0 ? "" : "row-" + Long.toUnsignedString(raw, 36); + // Arbitrary bytes, not UTF-8, of length 0..7: the raw's low bytes + case DType.Binary _ -> HEX.formatHex(ByteBuffer.allocate(8).putLong(raw).array(), 0, (int) (raw & 7)); + // The raw's low digits, so the shapes (runs, small ranges, constants) carry over + case DType.Decimal d -> BigDecimal.valueOf(raw % (long) Math.pow(10, Math.min(d.precision(), 18)), d.scale()); case DType.Primitive p -> switch (p.ptype()) { case I8 -> (long) (byte) raw; case I16 -> (long) (short) raw; @@ -200,6 +212,12 @@ private static Object writerValues(DType dtype, List canonical) { if (dtype instanceof DType.Utf8) { return canonical.toArray(new String[0]); } + if (dtype instanceof DType.Decimal) { + return canonical.toArray(new BigDecimal[0]); + } + if (dtype instanceof DType.Binary) { + return canonical.stream().map(v -> v == null ? null : HEX.parseHex((String) v)).toArray(byte[][]::new); + } boolean[] valid = new boolean[n]; long[] bits = new long[n]; boolean[] bools = new boolean[n]; @@ -316,6 +334,8 @@ private static void appendJava(Chunk chunk, Column column, List out) { private static Object javaValue(Array values, long i, DType dtype) { Object raw = switch (values) { + case DecimalArray a -> a.getDecimal(i); + case VarBinArray a when dtype instanceof DType.Binary -> HEX.formatHex(a.getBytes(i)); case LongArray a -> a.getLong(i); case IntArray a -> (long) a.getInt(i); case ShortArray a -> (long) a.getShort(i); @@ -357,6 +377,8 @@ private static Object jniValue(Object value, DType dtype) { case Float f -> Float.floatToIntBits(f); case Boolean b -> b; case Character c -> (long) c; + case BigDecimal d -> d; + case byte[] b -> HEX.formatHex(b); case Number n -> zeroExtend(dtype, n.longValue()); case org.apache.arrow.vector.util.Text text -> text.toString(); default -> value.toString(); diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/PostscriptParser.java b/reader/src/main/java/io/github/dfa1/vortex/reader/PostscriptParser.java index bfb91ce3a..84f10bba1 100644 --- a/reader/src/main/java/io/github/dfa1/vortex/reader/PostscriptParser.java +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/PostscriptParser.java @@ -265,21 +265,8 @@ private static DType convertDType(io.github.dfa1.vortex.core.fbs.FbsDType fbs) { } case FbsType.FbsDecimal -> { var d = fbs.type(new FbsDecimal()); - int precision = d.precision(); - int scale = d.scale(); - // Rust's DecimalDType::try_new bounds: precision 1..76 (i256), and a positive scale no - // greater than the precision; a negative scale (e.g. -2 for hundreds) is legal. Reject - // crafted values up front rather than letting a downstream BigDecimal/byte-width - // calculation fail with an unrelated exception. - if (precision < 1 || precision > 76) { - throw new VortexException( - "decimal precision " + precision + " out of range (expected 1..76)"); - } - if (scale > precision) { - throw new VortexException( - "decimal scale " + scale + " exceeds precision " + precision); - } - yield new DType.Decimal((byte) precision, (byte) scale, d.nullable()); + // DType.Decimal.of enforces Rust's DecimalDType::try_new bounds as VortexException + yield DType.Decimal.of(d.precision(), d.scale(), d.nullable()); } case FbsType.FbsUtf8 -> new DType.Utf8(fbs.type(new FbsUtf8()).nullable()); case FbsType.FbsBinary -> new DType.Binary(fbs.type(new FbsBinary()).nullable()); diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/decode/VariantEncodingDecoder.java b/reader/src/main/java/io/github/dfa1/vortex/reader/decode/VariantEncodingDecoder.java index 4747e81dd..0deb31d35 100644 --- a/reader/src/main/java/io/github/dfa1/vortex/reader/decode/VariantEncodingDecoder.java +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/decode/VariantEncodingDecoder.java @@ -85,9 +85,9 @@ static DType dtypeFromProto(io.github.dfa1.vortex.core.proto.ProtoDType proto) { proto.primitive().nullable()); } if (proto.decimal() != null) { - return new DType.Decimal( - (byte) proto.decimal().precision(), - (byte) proto.decimal().scale(), + return DType.Decimal.of( + proto.decimal().precision(), + proto.decimal().scale(), proto.decimal().nullable()); } if (proto.utf8() != null) { diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/FrameOfReferenceEncodingEncoder.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/FrameOfReferenceEncodingEncoder.java index 00e391bb2..692c778df 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/FrameOfReferenceEncodingEncoder.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/FrameOfReferenceEncodingEncoder.java @@ -58,7 +58,7 @@ public EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { long[] longs = PrimitiveArrays.toLongs(data, ptype, EncodingId.FASTLANES_FOR); int n = longs.length; - long ref = computeRef(longs, n); + long ref = computeRef(longs, n, ptype); MemorySegment residuals = toResidualBuffer(longs, ref, ptype, ctx); MemorySegment meta = buildForMeta(ref, ptype); @@ -78,7 +78,7 @@ public CascadeStep encodeCascade(DType dtype, Object data, EncodeContext encodeC long[] longs = PrimitiveArrays.toLongs(data, ptype, EncodingId.FASTLANES_FOR); int n = longs.length; - long ref = computeRef(longs, n); + long ref = computeRef(longs, n, ptype); // Skip when ref == 0 and ptype is unsigned: residuals == input, so FOR adds metadata // overhead (ref scalar + extra node) for zero compression benefit over plain bitpack. // Matches Rust IntFoRScheme's skip estimate for this case. @@ -95,8 +95,11 @@ public CascadeStep encodeCascade(DType dtype, Object data, EncodeContext encodeC PrimitiveEncodingEncoder.minOf(stats), PrimitiveEncodingEncoder.maxOf(stats), true); } - private static long computeRef(long[] longs, int n) { - return n > 0 ? SimdOperationsSupport.preferred().minMax(longs, PType.I64)[0] : 0L; + // Rust takes the reference from Stat::Min, an unsigned min for unsigned ptypes: a signed one + // picks a u64 above 2^63 over a smaller value below it, so a column straddling 2^63 got full-width + // residuals. Widened U8..U32 values are zero-extended, so the U64 order serves every unsigned width. + private static long computeRef(long[] longs, int n, PType ptype) { + return n > 0 ? SimdOperationsSupport.preferred().minMax(longs, ptype.isUnsigned() ? PType.U64 : PType.I64)[0] : 0L; } private static MemorySegment buildForMeta(long ref, PType ptype) { diff --git a/writer/src/test/java/io/github/dfa1/vortex/writer/encode/FrameOfReferenceEncodingEncoderTest.java b/writer/src/test/java/io/github/dfa1/vortex/writer/encode/FrameOfReferenceEncodingEncoderTest.java index cc5667e6f..622d459fa 100644 --- a/writer/src/test/java/io/github/dfa1/vortex/writer/encode/FrameOfReferenceEncodingEncoderTest.java +++ b/writer/src/test/java/io/github/dfa1/vortex/writer/encode/FrameOfReferenceEncodingEncoderTest.java @@ -265,6 +265,22 @@ void encode_unsignedU32_statsUseUnsignedField() throws IOException { assertThat(result.statsMax().uint64_value()).isEqualTo(300L); } + @Test + void encode_u64StraddlingTwoToThe63_referenceIsUnsignedMin() throws IOException { + // Given values just below and above 2^63: a signed min picks 2^63 (Long.MIN_VALUE), + // leaving 2^63 - 10 a full-width residual, where Rust's Stat::Min picks 2^63 - 10 + long below = Long.MAX_VALUE - 9; + long[] data = {Long.MIN_VALUE + 5, below, Long.MIN_VALUE}; + + // When + EncodeResult result = ENCODER.encode(new DType.Primitive(PType.U64, false), data, EncodeTestHelper.testCtx()); + + // Then + MemorySegment meta = result.rootNode().metadata(); + ProtoScalarValue reference = ProtoScalarValue.decode(meta, 0, meta.byteSize()); + assertThat(reference.uint64_value()).isEqualTo(below); + } + @Test void encodeCascade_signedI64_statsCarryMinAndMax() throws IOException { // Given — same #379 regression as encode_signedI64_statsCarryMinAndMax, but for the