From 1dc2bac70a553ed848065f4693c7ca1d0ca71564 Mon Sep 17 00:00:00 2001 From: Davide Angelocola Date: Mon, 6 Jul 2026 22:33:55 +0200 Subject: [PATCH] fix: unsigned dtype handling in TUI, filter predicates, and Calcite (#216) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #208 (CSV export): the remaining consumers still read unsigned integer columns through raw signed getters, so high-half values (bit set at the width's sign position) were mishandled. Per site: - cli tui GridRender / InspectorRender: U64/U32 now render via Long/Integer.toUnsignedString; U16/U8 render via the dtype-aware getInt (zero-extend). Display bug only, but visible on any unsigned column. - cli FilterCommand.compareValue: the worst class — silent wrong query results. Filter predicates now widen U8/U16/U32 losslessly into a non-negative long and compare U64 with Long.compareUnsigned, so `magnesium >= 130` matches its high-half U8 rows instead of dropping them. Made package-private + unit-tested. - calcite VortexTable: unsigned columns now map to the next wider signed SQL type so the declared type holds their full range (U8->SMALLINT, U16->INTEGER, U32->BIGINT) and value() widens accordingly. U64 has no wider SQL integer, so it stays BIGINT and value() throws VortexException for values >= 2^63 rather than surfacing a two's-complement negative. - calcite VortexAggregates.scanSum: U32 elements widen via toUnsignedLong; U64 elements >= 2^63 throw (aligned with the VortexTable decision). Branch-split on the loop-invariant unsigned flag keeps the signed body vectorizable. U64 decision: mapped to signed BIGINT because Calcite has no wider SQL integer; a documented VortexException for the unrepresentable high half is chosen over a silent negative (loud beats wrong) — both when a row is read and when it is summed. Closes #216 Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 1 + .../dfa1/vortex/calcite/VortexAggregates.java | 37 ++++- .../dfa1/vortex/calcite/VortexTable.java | 44 +++++- .../vortex/calcite/UnsignedColumnTest.java | 132 ++++++++++++++++++ .../calcite/VortexAdapterCoverageTest.java | 10 +- .../github/dfa1/vortex/cli/FilterCommand.java | 51 +++++-- .../dfa1/vortex/cli/tui/GridRender.java | 26 +++- .../dfa1/vortex/cli/tui/InspectorRender.java | 25 +++- .../dfa1/vortex/cli/FilterCommandTest.java | 92 ++++++++++++ .../dfa1/vortex/cli/tui/ArrayFixtures.java | 36 +++++ .../dfa1/vortex/cli/tui/GridRenderTest.java | 23 +++ .../vortex/cli/tui/InspectorRenderTest.java | 17 +++ 12 files changed, 463 insertions(+), 31 deletions(-) create mode 100644 calcite/src/test/java/io/github/dfa1/vortex/calcite/UnsignedColumnTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 3dba4d57..eb72ca91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Scanning a struct whose columns include a nested struct no longer fails chunk planning. A nested `vortex.struct` layout column (e.g. Raincloud `countries-of-the-world`'s `data` column) is now treated as a single full-range chunk source and decoded through the layout registry's new struct decoder into a `StructArray`, matching the Rust reference (a nested struct spans its parent's row range, not an independent chunking). `schema`/`count`/`inspect` already worked; plain scans and `select` of sibling columns now work too. CSV export of the struct column itself remains unsupported (a separate rendering limitation). ([#207](https://github.com/dfa1/vortex-java/issues/207)) - CSV export renders unsigned integer columns (U8–U64) with their unsigned values — high-half values previously printed as two's-complement negatives (uci-wine `magnesium` U8 132 exported as -124), silent corruption found by the Raincloud conformance suite. ([#208](https://github.com/dfa1/vortex-java/issues/208)) +- Unsigned integer columns are handled unsigned in the remaining consumers: the CLI `filter` predicates (`magnesium >= 130` now matches its high-half U8 rows instead of silently dropping them — the worst class of the bug: wrong query results), the TUI grid and inspector views, and the Calcite adapter (U8/U16/U32 map to the next wider signed SQL type so their full range fits; U64 stays `BIGINT` and fails loud rather than surfacing a negative for values ≥ 2^63, both when read and when summed). ([#216](https://github.com/dfa1/vortex-java/issues/216)) - `fastlanes.rle` decodes F64/F32 value pools (`LazyRleDoubleArray`, `LazyRleFloatArray`) — files from the Python bindings RLE-encode double columns with long constant runs, which previously failed to scan with `unsupported ptype F64`. ([#209](https://github.com/dfa1/vortex-java/issues/209)) - Lazy dict decode now covers I8/U8/I16/U16 value columns (`DictByteArray`, `DictShortArray`) — files from the Python bindings dict-encode narrow-integer columns, which previously failed to scan with `unsupported ptype for lazy dict`. ([#206](https://github.com/dfa1/vortex-java/issues/206)) - CSV export handles all-null (`DType.Null`) columns as empty fields instead of throwing `unsupported array type: NullArray`. ([#211](https://github.com/dfa1/vortex-java/issues/211)) diff --git a/calcite/src/main/java/io/github/dfa1/vortex/calcite/VortexAggregates.java b/calcite/src/main/java/io/github/dfa1/vortex/calcite/VortexAggregates.java index edf2aff3..b124334c 100644 --- a/calcite/src/main/java/io/github/dfa1/vortex/calcite/VortexAggregates.java +++ b/calcite/src/main/java/io/github/dfa1/vortex/calcite/VortexAggregates.java @@ -1,6 +1,8 @@ package io.github.dfa1.vortex.calcite; +import io.github.dfa1.vortex.core.error.VortexException; import io.github.dfa1.vortex.core.model.ColumnName; +import io.github.dfa1.vortex.core.model.DType; import io.github.dfa1.vortex.reader.ArrayStats; import io.github.dfa1.vortex.reader.Chunk; import io.github.dfa1.vortex.reader.ScanIterator; @@ -83,6 +85,10 @@ public static Summary of(VortexReader reader, String column) { Source.ZONE_STATS_PUSHDOWN, sumSource); } + private static boolean isUnsigned(Array arr) { + return arr.dtype() instanceof DType.Primitive p && p.ptype().isUnsigned(); + } + private static long totalRows(VortexReader reader) { try (ScanIterator scan = reader.scan(ScanOptions.all())) { long total = 0L; @@ -103,13 +109,36 @@ private static Number scanSum(VortexReader reader, String column) { long n = chunk.rowCount(); switch (chunk.column(column)) { case LongArray a -> { - for (long i = 0; i < n; i++) { - longSum += a.getLong(i); + // Branch-split on the loop-invariant unsigned flag so the signed body + // stays vectorizable. A U64 element with the high bit set (>= 2^63) has + // no signed-long home and would corrupt the sum, so fail loud — aligned + // with VortexTable mapping U64 to signed BIGINT. + if (isUnsigned(a)) { + for (long i = 0; i < n; i++) { + long v = a.getLong(i); + if (v < 0) { + throw new VortexException("U64 value " + Long.toUnsignedString(v) + + " exceeds the signed BIGINT range SUM accumulates into"); + } + longSum += v; + } + } else { + for (long i = 0; i < n; i++) { + longSum += a.getLong(i); + } } } case IntArray a -> { - for (long i = 0; i < n; i++) { - longSum += a.getInt(i); + // U32 widens losslessly into the signed accumulator via toUnsignedLong; + // branch-split keeps the signed body free of the per-element widen. + if (isUnsigned(a)) { + for (long i = 0; i < n; i++) { + longSum += Integer.toUnsignedLong(a.getInt(i)); + } + } else { + for (long i = 0; i < n; i++) { + longSum += a.getInt(i); + } } } case DoubleArray a -> { diff --git a/calcite/src/main/java/io/github/dfa1/vortex/calcite/VortexTable.java b/calcite/src/main/java/io/github/dfa1/vortex/calcite/VortexTable.java index bc02ce25..8824be9c 100644 --- a/calcite/src/main/java/io/github/dfa1/vortex/calcite/VortexTable.java +++ b/calcite/src/main/java/io/github/dfa1/vortex/calcite/VortexTable.java @@ -1,5 +1,6 @@ package io.github.dfa1.vortex.calcite; +import io.github.dfa1.vortex.core.error.VortexException; import io.github.dfa1.vortex.core.model.ColumnName; import io.github.dfa1.vortex.core.model.DType; import io.github.dfa1.vortex.reader.ArrayStats; @@ -809,10 +810,17 @@ private static Object value(Object array, DType type, long r) { case DType.Primitive p -> switch (p.ptype()) { case F64 -> ((DoubleArray) array).getDouble(r); case F32 -> (double) ((FloatArray) array).getFloat(r); - case I64, U64 -> ((LongArray) array).getLong(r); + case I64 -> ((LongArray) array).getLong(r); + // U64 maps to signed BIGINT (no wider SQL integer exists): values with the high bit + // set have no signed-long representation, so fail loud rather than surface a + // negative to SQL — silent wrong results are the worse outcome. + case U64 -> unsignedBigint(((LongArray) array).getLong(r)); + // U32 exceeds signed INTEGER, so it maps to BIGINT and widens losslessly here. + case U32 -> Integer.toUnsignedLong(((IntArray) array).getInt(r)); // Narrow ints decode to their own array width (Byte/Short/Int), not IntArray — - // each exposes getInt(r) with the correct sign/zero extension. - case I32, U32 -> ((IntArray) array).getInt(r); + // each exposes getInt(r) with the correct sign/zero extension. U16/U8 zero-extend + // into the wider signed SQL type (INTEGER/SMALLINT) they map to. + case I32 -> ((IntArray) array).getInt(r); case I16, U16 -> ((ShortArray) array).getInt(r); case I8, U8 -> ((ByteArray) array).getInt(r); default -> throw new IllegalStateException("unsupported ptype: " + p.ptype()); @@ -823,6 +831,20 @@ private static Object value(Object array, DType type, long r) { }; } + /// Guards a U64 value being surfaced to Calcite's signed `BIGINT`: values with the high bit set + /// (`>= 2^63`) have no lossless signed-long representation, so this throws rather than let a + /// negative reach SQL. Vortex has no wider unsigned SQL type to widen into, so loud beats wrong. + /// + /// @param raw the raw U64 bits read from the column + /// @return `raw` unchanged when it fits the non-negative signed-long range + private static long unsignedBigint(long raw) { + if (raw < 0) { + throw new VortexException("U64 value " + Long.toUnsignedString(raw) + + " exceeds the signed BIGINT range Calcite maps U64 to"); + } + return raw; + } + /// Translates the Calcite predicates into a zone-map [RowFilter], keeping only the comparisons /// we can prune on (`=`, `<>`, `<`, `<=`, `>`, `>=` between a column and a literal, plus `AND`). /// Predicates we don't understand are simply not pushed — Calcite still applies them. @@ -966,15 +988,23 @@ private static Object literalValue(RexLiteral lit, DType type) { }; } + /// Maps a Vortex dtype to a Calcite SQL type. Unsigned integers map to the next wider signed + /// SQL type so the declared type can hold their full range (`U8`->`SMALLINT`, `U16`->`INTEGER`, + /// `U32`->`BIGINT`); `U64` is the exception — no wider SQL integer exists, so it maps to + /// `BIGINT` and [#value(Object, DType, long)] guards the high-half values that cannot fit. + /// + /// @param factory the Calcite type factory + /// @param type the Vortex column dtype + /// @return the nullable-adjusted SQL type private static RelDataType toSqlType(RelDataTypeFactory factory, DType type) { RelDataType sql = switch (type) { case DType.Primitive p -> switch (p.ptype()) { case F64 -> factory.createSqlType(SqlTypeName.DOUBLE); case F32 -> factory.createSqlType(SqlTypeName.REAL); - case I64, U64 -> factory.createSqlType(SqlTypeName.BIGINT); - case I32, U32 -> factory.createSqlType(SqlTypeName.INTEGER); - case I16, U16 -> factory.createSqlType(SqlTypeName.SMALLINT); - case I8, U8 -> factory.createSqlType(SqlTypeName.TINYINT); + case I64, U64, U32 -> factory.createSqlType(SqlTypeName.BIGINT); + case I32, U16 -> factory.createSqlType(SqlTypeName.INTEGER); + case I16, U8 -> factory.createSqlType(SqlTypeName.SMALLINT); + case I8 -> factory.createSqlType(SqlTypeName.TINYINT); default -> throw new IllegalStateException("unsupported ptype: " + p.ptype()); }; case DType.Utf8 _ -> factory.createSqlType(SqlTypeName.VARCHAR); diff --git a/calcite/src/test/java/io/github/dfa1/vortex/calcite/UnsignedColumnTest.java b/calcite/src/test/java/io/github/dfa1/vortex/calcite/UnsignedColumnTest.java new file mode 100644 index 00000000..55e60659 --- /dev/null +++ b/calcite/src/test/java/io/github/dfa1/vortex/calcite/UnsignedColumnTest.java @@ -0,0 +1,132 @@ +package io.github.dfa1.vortex.calcite; + +import io.github.dfa1.vortex.core.error.VortexException; +import io.github.dfa1.vortex.core.model.ColumnName; +import io.github.dfa1.vortex.core.model.DType; +import io.github.dfa1.vortex.reader.ReadRegistry; +import io.github.dfa1.vortex.reader.VortexReader; +import io.github.dfa1.vortex.writer.VortexWriter; +import io.github.dfa1.vortex.writer.WriteOptions; + +import org.apache.calcite.linq4j.Enumerator; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.channels.FileChannel; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/// Unsigned high-half handling in the Calcite adapter (#216): U32 widens losslessly into `BIGINT` +/// and U64 — mapped to signed `BIGINT` because no wider SQL integer exists — fails loud rather than +/// surfacing a two's-complement negative to SQL. The canonical motivating shape is an unsigned +/// integer column (like uci-wine's magnesium); here each column carries a boundary high-half value. +class UnsignedColumnTest { + + // A U32 value beyond signed INTEGER (0xB2D05E00): getInt reads it as a negative int. + private static final long U32_HIGH = 3_000_000_000L; + // A U64 value with the high bit set (2^63): getLong reads it as Long.MIN_VALUE. + private static final long U64_HIGH = Long.MIN_VALUE; + + private static final DType.Struct SCHEMA = DType.structBuilder() + .field("u32", DType.U32) + .field("u64", DType.U64) + .build(); + + @TempDir + Path tmp; + + @Test + void scan_widensU32ToLongAndKeepsInRangeU64() throws Exception { + // Given — a row whose U32 exceeds signed INTEGER and whose U64 stays in the low half + Path file = write("in-range.vortex", WriteOptions.defaults(), (int) U32_HIGH, 4000L); + + // When — a full scan materializes the row through VortexTable.value + List rows = drain(new VortexTable(file)); + + // Then — U32 widened to a positive Long (not a negative int), U64 kept as its Long value + assertThat(rows).hasSize(1); + assertThat(rows.getFirst()[0]).isEqualTo(U32_HIGH); + assertThat(rows.getFirst()[1]).isEqualTo(4000L); + } + + @Test + void scan_u64HighHalf_failsLoudRatherThanSurfaceNegative() throws Exception { + // Given — a U64 value with no lossless signed-BIGINT representation + Path file = write("u64-high.vortex", WriteOptions.defaults(), 0, U64_HIGH); + VortexTable table = new VortexTable(file); + + // When / Then — materialization throws instead of yielding a negative BIGINT + assertThatThrownBy(() -> drain(table)) + .isInstanceOf(VortexException.class) + .hasMessageContaining("U64 value") + .hasMessageContaining("BIGINT"); + } + + @Test + void sum_u32HighHalf_widensExactlyOnFullScan() throws Exception { + // Given — zone maps off forces the streaming scanSum path (no per-zone SUM to fold) + Path file = write("sum-u32.vortex", noZoneMaps(), (int) U32_HIGH, 1L); + + // When + try (VortexReader reader = VortexReader.open(file, registry())) { + VortexAggregates.Summary result = VortexAggregates.of(reader, "u32"); + + // Then — the U32 element widens via toUnsignedLong, not sign-extension + assertThat(result.sumSource()).isEqualTo(VortexAggregates.Source.FULL_SCAN); + assertThat(result.sum()).isInstanceOf(Long.class).isEqualTo(U32_HIGH); + } + } + + @Test + void sum_u64HighHalf_failsLoudOnFullScan() throws Exception { + // Given — a U64 element the signed accumulator cannot represent, zone maps off + Path file = write("sum-u64.vortex", noZoneMaps(), 0, U64_HIGH); + + // When / Then — scanSum throws rather than corrupt the sum with a negative addend + try (VortexReader reader = VortexReader.open(file, registry())) { + assertThatThrownBy(() -> VortexAggregates.of(reader, "u64")) + .isInstanceOf(VortexException.class) + .hasMessageContaining("U64 value"); + } + } + + private Path write(String name, WriteOptions opts, int u32, long u64) throws Exception { + Path file = tmp.resolve(name); + try (FileChannel ch = FileChannel.open(file, StandardOpenOption.CREATE, StandardOpenOption.WRITE); + VortexWriter writer = VortexWriter.create(ch, SCHEMA, opts)) { + writer.writeChunk(Map.of( + ColumnName.of("u32"), new int[]{u32}, + ColumnName.of("u64"), new long[]{u64})); + } + return file; + } + + private static WriteOptions noZoneMaps() { + // Same shape as the adapter coverage test's zone-maps-off options: the second flag disables + // zone maps so no per-zone SUM exists and VortexAggregates falls back to scanSum. + return new WriteOptions(65_536, false, 0.90, 0, true, false); + } + + private static ReadRegistry registry() { + return ReadRegistry.builder().registerServiceLoaded().build(); + } + + private static List drain(VortexTable table) { + List rows = new ArrayList<>(); + Enumerator en = table.scan(null, List.of(), null).enumerator(); + try { + while (en.moveNext()) { + rows.add(en.current()); + } + } finally { + en.close(); + } + return rows; + } +} diff --git a/calcite/src/test/java/io/github/dfa1/vortex/calcite/VortexAdapterCoverageTest.java b/calcite/src/test/java/io/github/dfa1/vortex/calcite/VortexAdapterCoverageTest.java index 50087ed8..2ac64030 100644 --- a/calcite/src/test/java/io/github/dfa1/vortex/calcite/VortexAdapterCoverageTest.java +++ b/calcite/src/test/java/io/github/dfa1/vortex/calcite/VortexAdapterCoverageTest.java @@ -82,11 +82,13 @@ void getRowType_mapsEveryColumnToItsSqlType() { // Given / When RelDataType rowType = new VortexTable(file).getRowType(new JavaTypeFactoryImpl()); - // Then — one SqlTypeName per logical type + // Then — one SqlTypeName per logical type. Unsigned integers map to the next wider signed + // SQL type so the declared type holds their full range (#216); U64 is the exception (no + // wider SQL integer) and stays BIGINT with a value-side overflow guard. Map expected = Map.ofEntries( - Map.entry("i8", SqlTypeName.TINYINT), Map.entry("u8", SqlTypeName.TINYINT), - Map.entry("i16", SqlTypeName.SMALLINT), Map.entry("u16", SqlTypeName.SMALLINT), - Map.entry("i32", SqlTypeName.INTEGER), Map.entry("u32", SqlTypeName.INTEGER), + Map.entry("i8", SqlTypeName.TINYINT), Map.entry("u8", SqlTypeName.SMALLINT), + Map.entry("i16", SqlTypeName.SMALLINT), Map.entry("u16", SqlTypeName.INTEGER), + Map.entry("i32", SqlTypeName.INTEGER), Map.entry("u32", SqlTypeName.BIGINT), Map.entry("i64", SqlTypeName.BIGINT), Map.entry("u64", SqlTypeName.BIGINT), Map.entry("f32", SqlTypeName.REAL), Map.entry("f64", SqlTypeName.DOUBLE), Map.entry("s", SqlTypeName.VARCHAR), Map.entry("b", SqlTypeName.BOOLEAN)); diff --git a/cli/src/main/java/io/github/dfa1/vortex/cli/FilterCommand.java b/cli/src/main/java/io/github/dfa1/vortex/cli/FilterCommand.java index 6bd49cf5..a064abfe 100644 --- a/cli/src/main/java/io/github/dfa1/vortex/cli/FilterCommand.java +++ b/cli/src/main/java/io/github/dfa1/vortex/cli/FilterCommand.java @@ -1,5 +1,6 @@ package io.github.dfa1.vortex.cli; +import io.github.dfa1.vortex.core.model.DType; import io.github.dfa1.vortex.reader.array.Array; import io.github.dfa1.vortex.reader.array.BoolArray; import io.github.dfa1.vortex.reader.array.ByteArray; @@ -198,12 +199,29 @@ private static boolean isRowNull(Array arr, long rowIdx) { return arr instanceof MaskedArray masked && !masked.isValid(rowIdx); } - private static int compareValue(Array arr, long rowIdx, Comparable value) { + /// Compares the row's column value against the filter literal. Package-private so the + /// per-dtype arms — in particular the unsigned paths, which the CLI grammar can only reach + /// with a real file — can be exercised directly against in-memory arrays. + /// + /// @param arr the decoded column array + /// @param rowIdx zero-based row index into `arr` + /// @param value the parsed filter literal ([Long], [Double], [Boolean], or [String]) + /// @return a negative, zero, or positive int as the column value orders before, equal to, or + /// after the literal + static int compareValue(Array arr, long rowIdx, Comparable value) { return switch (arr) { - case LongArray la -> compareNumeric(la.getLong(rowIdx), value); - case IntArray ia -> compareNumeric(ia.getInt(rowIdx), value); - case ShortArray sa -> compareNumeric(sa.getShort(rowIdx), value); - case ByteArray ba -> compareNumeric(ba.getByte(rowIdx), value); + // U64 has no lossless signed-long home: keep the raw bits and compare unsigned when the + // dtype is unsigned, so high-half values (>= 2^63, which getLong returns as negative) + // order after the low half instead of before it. Silent wrong query results otherwise. + case LongArray la -> compareNumeric(la.getLong(rowIdx), isUnsigned(la), value); + // Byte/Short/Int widen losslessly into a non-negative long — U8/U16 via their + // dtype-aware getInt (zero-extend), U32 via toUnsignedLong — so a plain signed compare + // of the widened value is exact. + case IntArray ia -> compareNumeric(isUnsigned(ia) + ? Integer.toUnsignedLong(ia.getInt(rowIdx)) + : ia.getInt(rowIdx), false, value); + case ShortArray sa -> compareNumeric(sa.getInt(rowIdx), false, value); + case ByteArray ba -> compareNumeric(ba.getInt(rowIdx), false, value); case DoubleArray da -> compareDouble(da.getDouble(rowIdx), value); case FloatArray fa -> compareDouble(fa.getFloat(rowIdx), value); case BoolArray ba -> Boolean.compare(ba.getBoolean(rowIdx), (Boolean) value); @@ -216,11 +234,28 @@ private static int compareValue(Array arr, long rowIdx, Comparable value) { }; } - private static int compareNumeric(long colVal, Comparable value) { + private static boolean isUnsigned(Array arr) { + return arr.dtype() instanceof DType.Primitive p && p.ptype().isUnsigned(); + } + + private static int compareNumeric(long colVal, boolean unsigned, Comparable value) { if (value instanceof Long l) { - return Long.compare(colVal, l); + return unsigned ? Long.compareUnsigned(colVal, l) : Long.compare(colVal, l); } - return Double.compare(colVal, (Double) value); + return Double.compare(unsigned ? unsignedToDouble(colVal) : colVal, (Double) value); + } + + /// Converts a possibly-unsigned long to the nearest double without letting the sign bit flip it + /// negative. For values `<= Long.MAX_VALUE` this is exact; above it the standard split keeps the + /// magnitude correct. + /// + /// @param v the raw long, interpreted as unsigned + /// @return `v` as an unsigned magnitude in `double` + private static double unsignedToDouble(long v) { + if (v >= 0) { + return v; + } + return ((double) (v >>> 1)) * 2.0 + (v & 1L); } private static int compareDouble(double colVal, Comparable value) { diff --git a/cli/src/main/java/io/github/dfa1/vortex/cli/tui/GridRender.java b/cli/src/main/java/io/github/dfa1/vortex/cli/tui/GridRender.java index 8423a6e1..7274b8a3 100644 --- a/cli/src/main/java/io/github/dfa1/vortex/cli/tui/GridRender.java +++ b/cli/src/main/java/io/github/dfa1/vortex/cli/tui/GridRender.java @@ -58,10 +58,19 @@ static String formatCell(Array array, long i, DType declared) { } } return switch (inner) { - case LongArray a -> Long.toString(a.getLong(i)); - case IntArray a -> Integer.toString(a.getInt(i)); - case ShortArray a -> Short.toString(a.getShort(i)); - case ByteArray a -> Byte.toString(a.getByte(i)); + // Long/IntArray have no dtype-aware getter, so gate the unsigned rendering on the + // ptype; a U64/U32 high-half value would otherwise show as a negative decimal + // (e.g. a U8 magnesium column widened to U64 with the sign bit set). + case LongArray a -> isUnsigned(a) + ? Long.toUnsignedString(a.getLong(i)) + : Long.toString(a.getLong(i)); + case IntArray a -> isUnsigned(a) + ? Integer.toUnsignedString(a.getInt(i)) + : Integer.toString(a.getInt(i)); + // Short/ByteArray.getInt already zero-extends U16/U8, so widening to int and + // printing signed decimal yields the correct unsigned value. + case ShortArray a -> Integer.toString(a.getInt(i)); + case ByteArray a -> Integer.toString(a.getInt(i)); case DoubleArray a -> Double.toString(a.getDouble(i)); case FloatArray a -> Float.toString(a.getFloat(i)); case BoolArray a -> Boolean.toString(a.getBoolean(i)); @@ -78,6 +87,15 @@ static String formatCell(Array array, long i, DType declared) { } } + /// Whether the array's dtype is an unsigned integer, so high-half values must render as + /// unsigned decimal rather than the two's-complement negative the raw signed getter returns. + /// + /// @param arr the leaf value array (never a [MaskedArray]; the caller unwraps first) + /// @return `true` when the dtype is a `U8`–`U64` primitive + private static boolean isUnsigned(Array arr) { + return arr.dtype() instanceof DType.Primitive p && p.ptype().isUnsigned(); + } + private static Optional formatExtension(DType.Extension ext, Array storage, long i) { Optional idOpt = ExtensionId.parse(ext.extensionId()); if (idOpt.isEmpty()) { diff --git a/cli/src/main/java/io/github/dfa1/vortex/cli/tui/InspectorRender.java b/cli/src/main/java/io/github/dfa1/vortex/cli/tui/InspectorRender.java index e2c2b581..b96b3d36 100644 --- a/cli/src/main/java/io/github/dfa1/vortex/cli/tui/InspectorRender.java +++ b/cli/src/main/java/io/github/dfa1/vortex/cli/tui/InspectorRender.java @@ -48,10 +48,18 @@ static String formatValue(Array array, int i, DType declared) { } } return switch (array) { - case LongArray a -> Long.toString(a.getLong(i)); - case IntArray a -> Integer.toString(a.getInt(i)); - case ShortArray a -> Short.toString(a.getShort(i)); - case ByteArray a -> Byte.toString(a.getByte(i)); + // Long/IntArray have no dtype-aware getter, so gate the unsigned rendering on the + // ptype; a U64/U32 high-half value would otherwise show as a negative decimal. + case LongArray a -> isUnsigned(a) + ? Long.toUnsignedString(a.getLong(i)) + : Long.toString(a.getLong(i)); + case IntArray a -> isUnsigned(a) + ? Integer.toUnsignedString(a.getInt(i)) + : Integer.toString(a.getInt(i)); + // Short/ByteArray.getInt already zero-extends U16/U8, so a signed-decimal print of the + // widened int is the correct unsigned value. + case ShortArray a -> Integer.toString(a.getInt(i)); + case ByteArray a -> Integer.toString(a.getInt(i)); case DoubleArray a -> Double.toString(a.getDouble(i)); case FloatArray a -> Float.toString(a.getFloat(i)); case BoolArray a -> Boolean.toString(a.getBoolean(i)); @@ -116,6 +124,15 @@ private static String formatStatsCell(Array field, int row, DType declared) { return formatValue(field, row, declared); } + /// Whether the array's dtype is an unsigned integer, so high-half values must render as + /// unsigned decimal rather than the two's-complement negative the raw signed getter returns. + /// + /// @param arr the value array under inspection + /// @return `true` when the dtype is a `U8`–`U64` primitive + private static boolean isUnsigned(Array arr) { + return arr.dtype() instanceof DType.Primitive p && p.ptype().isUnsigned(); + } + private static String tryDecimal(LongFunction reader, Array a, int i) { try { return reader.apply(i).toPlainString(); diff --git a/cli/src/test/java/io/github/dfa1/vortex/cli/FilterCommandTest.java b/cli/src/test/java/io/github/dfa1/vortex/cli/FilterCommandTest.java index d7e0b258..46074c66 100644 --- a/cli/src/test/java/io/github/dfa1/vortex/cli/FilterCommandTest.java +++ b/cli/src/test/java/io/github/dfa1/vortex/cli/FilterCommandTest.java @@ -1,8 +1,15 @@ package io.github.dfa1.vortex.cli; +import io.github.dfa1.vortex.core.model.DType; +import io.github.dfa1.vortex.reader.array.Array; +import io.github.dfa1.vortex.reader.array.MaterializedByteArray; +import io.github.dfa1.vortex.reader.array.MaterializedIntArray; +import io.github.dfa1.vortex.reader.array.MaterializedLongArray; +import io.github.dfa1.vortex.reader.array.MaterializedShortArray; import io.github.dfa1.vortex.reader.compute.Predicate; import io.github.dfa1.vortex.csv.RowPredicate; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.params.ParameterizedTest; @@ -11,6 +18,9 @@ import org.junit.jupiter.params.provider.ValueSource; import java.io.IOException; +import java.lang.foreign.Arena; +import java.lang.foreign.MemorySegment; +import java.lang.foreign.ValueLayout; import java.nio.file.Path; import java.util.stream.Stream; @@ -226,4 +236,86 @@ private static Stream unsupportedCompositePredicates() { new Predicate.And(leaf, leaf), new Predicate.Or(leaf, leaf)); } + + /// Direct tests of the unsigned comparison arms of [FilterCommand#compareValue]. The CLI + /// grammar can only reach these with a real file, so exercise them against in-memory arrays — + /// the bug they guard is silent wrong query results on unsigned columns (#216). + @Nested + class CompareValueUnsigned { + + @Test + void unsignedHighHalfOrdersAfterSmallLiteral() { + try (Arena arena = Arena.ofConfined()) { + // Given — the magnesium filter shape: an unsigned column carrying a high-half value + // that the old signed getters read as negative, silently ordering it BEFORE the + // literal (so `col >= 130` wrongly excluded it). Each width holds such a value. + Long threshold = 130L; + + // When / Then — every unsigned high-half value must compare greater than 130 + assertThat(FilterCommand.compareValue(u64(arena, Long.MIN_VALUE), 0, threshold)).isPositive(); + assertThat(FilterCommand.compareValue(u32(arena, (int) 3_000_000_000L), 0, threshold)).isPositive(); + assertThat(FilterCommand.compareValue(u16(arena, (short) 60_000), 0, threshold)).isPositive(); + assertThat(FilterCommand.compareValue(u8(arena, (byte) 200), 0, threshold)).isPositive(); + } + } + + @Test + void signedNegativeStillOrdersBeforeZero() { + try (Arena arena = Arena.ofConfined()) { + // Given — a signed column must keep its negative ordering: the unsigned path must + // not leak into signed dtypes. + // When / Then + assertThat(FilterCommand.compareValue(i64(arena, -1L), 0, 0L)).isNegative(); + assertThat(FilterCommand.compareValue(i8(arena, (byte) -5), 0, 0L)).isNegative(); + } + } + + @Test + void unsignedU64ComparedToDoubleLiteral() { + try (Arena arena = Arena.ofConfined()) { + // Given — a fractional literal takes the double path; a U64 high-half value must + // convert to a large positive magnitude, not a negative double. + // When / Then + assertThat(FilterCommand.compareValue(u64(arena, Long.MIN_VALUE), 0, 130.5)).isPositive(); + } + } + + private Array u64(Arena arena, long v) { + return new MaterializedLongArray(DType.U64, 1, longSeg(arena, v)); + } + + private Array i64(Arena arena, long v) { + return new MaterializedLongArray(DType.I64, 1, longSeg(arena, v)); + } + + private Array u32(Arena arena, int v) { + MemorySegment seg = arena.allocate(4, 4); + seg.set(ValueLayout.JAVA_INT, 0, v); + return new MaterializedIntArray(DType.U32, 1, seg.asReadOnly()); + } + + private Array u16(Arena arena, short v) { + MemorySegment seg = arena.allocate(2, 2); + seg.set(ValueLayout.JAVA_SHORT, 0, v); + return new MaterializedShortArray(DType.U16, 1, seg.asReadOnly()); + } + + private Array u8(Arena arena, byte v) { + MemorySegment seg = arena.allocate(1, 1); + seg.set(ValueLayout.JAVA_BYTE, 0, v); + return new MaterializedByteArray(DType.U8, 1, seg.asReadOnly()); + } + + private Array i8(Arena arena, byte v) { + MemorySegment seg = arena.allocate(1, 1); + seg.set(ValueLayout.JAVA_BYTE, 0, v); + return new MaterializedByteArray(DType.I8, 1, seg.asReadOnly()); + } + + private MemorySegment longSeg(Arena arena, long v) { + MemorySegment seg = arena.allocate(8, 8); + seg.set(ValueLayout.JAVA_LONG, 0, v); + return seg.asReadOnly(); + } + } } diff --git a/cli/src/test/java/io/github/dfa1/vortex/cli/tui/ArrayFixtures.java b/cli/src/test/java/io/github/dfa1/vortex/cli/tui/ArrayFixtures.java index 3f24dbf6..70413474 100644 --- a/cli/src/test/java/io/github/dfa1/vortex/cli/tui/ArrayFixtures.java +++ b/cli/src/test/java/io/github/dfa1/vortex/cli/tui/ArrayFixtures.java @@ -47,6 +47,42 @@ static IntArray ints(Arena arena, int... vs) { return new MaterializedIntArray(DType.I32, vs.length, seg.asReadOnly()); } + /// Builds a `U64` [LongArray]; high-half values (bit 63 set) exercise unsigned rendering. + static LongArray ulongs(Arena arena, long... vs) { + MemorySegment seg = arena.allocate(vs.length * 8L, 8); + for (int i = 0; i < vs.length; i++) { + seg.setAtIndex(ValueLayout.JAVA_LONG, i, vs[i]); + } + return new MaterializedLongArray(DType.U64, vs.length, seg.asReadOnly()); + } + + /// Builds a `U32` [IntArray]; values with bit 31 set exercise unsigned rendering. + static IntArray uints(Arena arena, int... vs) { + MemorySegment seg = arena.allocate(vs.length * 4L, 4); + for (int i = 0; i < vs.length; i++) { + seg.setAtIndex(ValueLayout.JAVA_INT, i, vs[i]); + } + return new MaterializedIntArray(DType.U32, vs.length, seg.asReadOnly()); + } + + /// Builds a `U16` [ShortArray]; values with bit 15 set exercise unsigned zero-extension. + static ShortArray ushorts(Arena arena, short... vs) { + MemorySegment seg = arena.allocate(vs.length * 2L, 2); + for (int i = 0; i < vs.length; i++) { + seg.setAtIndex(ValueLayout.JAVA_SHORT, i, vs[i]); + } + return new MaterializedShortArray(DType.U16, vs.length, seg.asReadOnly()); + } + + /// Builds a `U8` [ByteArray]; values with bit 7 set exercise unsigned zero-extension. + static ByteArray ubytes(Arena arena, byte... vs) { + MemorySegment seg = arena.allocate(vs.length, 1); + for (int i = 0; i < vs.length; i++) { + seg.set(ValueLayout.JAVA_BYTE, i, vs[i]); + } + return new MaterializedByteArray(DType.U8, vs.length, seg.asReadOnly()); + } + static ShortArray shorts(Arena arena, short... vs) { MemorySegment seg = arena.allocate(vs.length * 2L, 2); for (int i = 0; i < vs.length; i++) { diff --git a/cli/src/test/java/io/github/dfa1/vortex/cli/tui/GridRenderTest.java b/cli/src/test/java/io/github/dfa1/vortex/cli/tui/GridRenderTest.java index 6ecc9c98..cd051759 100644 --- a/cli/src/test/java/io/github/dfa1/vortex/cli/tui/GridRenderTest.java +++ b/cli/src/test/java/io/github/dfa1/vortex/cli/tui/GridRenderTest.java @@ -62,6 +62,29 @@ void rendersNumericAndBoolTypes() { } } + @Test + void rendersUnsignedHighHalfValuesAsUnsignedDecimal() { + try (Arena arena = Arena.ofConfined()) { + // Given — the canonical shape is an unsigned integer column (like uci-wine's magnesium, + // stored unsigned). Here each width carries a high-half value whose two's-complement + // signed reading is negative; the grid must show the unsigned decimal instead. + DType u64 = DType.U64; + DType u32 = DType.U32; + DType u16 = DType.U16; + DType u8 = DType.U8; + + // When / Then — U64 2^63, U32 3e9, U16 60000, U8 200 all render positive + assertThat(GridRender.formatCell(ArrayFixtures.ulongs(arena, Long.MIN_VALUE), 0, u64)) + .isEqualTo("9223372036854775808"); + assertThat(GridRender.formatCell(ArrayFixtures.uints(arena, (int) 3_000_000_000L), 0, u32)) + .isEqualTo("3000000000"); + assertThat(GridRender.formatCell(ArrayFixtures.ushorts(arena, (short) 60_000), 0, u16)) + .isEqualTo("60000"); + assertThat(GridRender.formatCell(ArrayFixtures.ubytes(arena, (byte) 200), 0, u8)) + .isEqualTo("200"); + } + } + @Test void rendersDecimal() { // Given diff --git a/cli/src/test/java/io/github/dfa1/vortex/cli/tui/InspectorRenderTest.java b/cli/src/test/java/io/github/dfa1/vortex/cli/tui/InspectorRenderTest.java index ee079ca3..9e16aa50 100644 --- a/cli/src/test/java/io/github/dfa1/vortex/cli/tui/InspectorRenderTest.java +++ b/cli/src/test/java/io/github/dfa1/vortex/cli/tui/InspectorRenderTest.java @@ -51,6 +51,23 @@ void rendersEachNumericAndBoolType() { } } + @Test + void rendersUnsignedHighHalfValuesAsUnsignedDecimal() { + try (Arena arena = Arena.ofConfined()) { + // Given — an unsigned integer column (uci-wine's magnesium is stored unsigned); the + // detail view must not surface the two's-complement negative for high-half values. + // When / Then — U64 2^63, U32 3e9, U16 60000, U8 200 render as unsigned decimals + assertThat(InspectorRender.formatValue(ArrayFixtures.ulongs(arena, Long.MIN_VALUE), 0, DType.U64)) + .isEqualTo("9223372036854775808"); + assertThat(InspectorRender.formatValue(ArrayFixtures.uints(arena, (int) 3_000_000_000L), 0, DType.U32)) + .isEqualTo("3000000000"); + assertThat(InspectorRender.formatValue(ArrayFixtures.ushorts(arena, (short) 60_000), 0, DType.U16)) + .isEqualTo("60000"); + assertThat(InspectorRender.formatValue(ArrayFixtures.ubytes(arena, (byte) 200), 0, DType.U8)) + .isEqualTo("200"); + } + } + @Test void rendersDecimal() { // Given