Skip to content

Commit 9af5a27

Browse files
dfa1claude
andcommitted
fix: unsigned dtype handling in TUI, filter predicates, and Calcite (#216)
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 <noreply@anthropic.com>
1 parent 0646243 commit 9af5a27

12 files changed

Lines changed: 463 additions & 31 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1111

1212
- 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))
1313
- 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))
14+
- 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))
1415
- `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))
1516
- 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))
1617
- 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))

calcite/src/main/java/io/github/dfa1/vortex/calcite/VortexAggregates.java

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package io.github.dfa1.vortex.calcite;
22

3+
import io.github.dfa1.vortex.core.error.VortexException;
34
import io.github.dfa1.vortex.core.model.ColumnName;
5+
import io.github.dfa1.vortex.core.model.DType;
46
import io.github.dfa1.vortex.reader.ArrayStats;
57
import io.github.dfa1.vortex.reader.Chunk;
68
import io.github.dfa1.vortex.reader.ScanIterator;
@@ -83,6 +85,10 @@ public static Summary of(VortexReader reader, String column) {
8385
Source.ZONE_STATS_PUSHDOWN, sumSource);
8486
}
8587

88+
private static boolean isUnsigned(Array arr) {
89+
return arr.dtype() instanceof DType.Primitive p && p.ptype().isUnsigned();
90+
}
91+
8692
private static long totalRows(VortexReader reader) {
8793
try (ScanIterator scan = reader.scan(ScanOptions.all())) {
8894
long total = 0L;
@@ -103,13 +109,36 @@ private static Number scanSum(VortexReader reader, String column) {
103109
long n = chunk.rowCount();
104110
switch (chunk.<Array>column(column)) {
105111
case LongArray a -> {
106-
for (long i = 0; i < n; i++) {
107-
longSum += a.getLong(i);
112+
// Branch-split on the loop-invariant unsigned flag so the signed body
113+
// stays vectorizable. A U64 element with the high bit set (>= 2^63) has
114+
// no signed-long home and would corrupt the sum, so fail loud — aligned
115+
// with VortexTable mapping U64 to signed BIGINT.
116+
if (isUnsigned(a)) {
117+
for (long i = 0; i < n; i++) {
118+
long v = a.getLong(i);
119+
if (v < 0) {
120+
throw new VortexException("U64 value " + Long.toUnsignedString(v)
121+
+ " exceeds the signed BIGINT range SUM accumulates into");
122+
}
123+
longSum += v;
124+
}
125+
} else {
126+
for (long i = 0; i < n; i++) {
127+
longSum += a.getLong(i);
128+
}
108129
}
109130
}
110131
case IntArray a -> {
111-
for (long i = 0; i < n; i++) {
112-
longSum += a.getInt(i);
132+
// U32 widens losslessly into the signed accumulator via toUnsignedLong;
133+
// branch-split keeps the signed body free of the per-element widen.
134+
if (isUnsigned(a)) {
135+
for (long i = 0; i < n; i++) {
136+
longSum += Integer.toUnsignedLong(a.getInt(i));
137+
}
138+
} else {
139+
for (long i = 0; i < n; i++) {
140+
longSum += a.getInt(i);
141+
}
113142
}
114143
}
115144
case DoubleArray a -> {

calcite/src/main/java/io/github/dfa1/vortex/calcite/VortexTable.java

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package io.github.dfa1.vortex.calcite;
22

3+
import io.github.dfa1.vortex.core.error.VortexException;
34
import io.github.dfa1.vortex.core.model.ColumnName;
45
import io.github.dfa1.vortex.core.model.DType;
56
import io.github.dfa1.vortex.reader.ArrayStats;
@@ -809,10 +810,17 @@ private static Object value(Object array, DType type, long r) {
809810
case DType.Primitive p -> switch (p.ptype()) {
810811
case F64 -> ((DoubleArray) array).getDouble(r);
811812
case F32 -> (double) ((FloatArray) array).getFloat(r);
812-
case I64, U64 -> ((LongArray) array).getLong(r);
813+
case I64 -> ((LongArray) array).getLong(r);
814+
// U64 maps to signed BIGINT (no wider SQL integer exists): values with the high bit
815+
// set have no signed-long representation, so fail loud rather than surface a
816+
// negative to SQL — silent wrong results are the worse outcome.
817+
case U64 -> unsignedBigint(((LongArray) array).getLong(r));
818+
// U32 exceeds signed INTEGER, so it maps to BIGINT and widens losslessly here.
819+
case U32 -> Integer.toUnsignedLong(((IntArray) array).getInt(r));
813820
// Narrow ints decode to their own array width (Byte/Short/Int), not IntArray —
814-
// each exposes getInt(r) with the correct sign/zero extension.
815-
case I32, U32 -> ((IntArray) array).getInt(r);
821+
// each exposes getInt(r) with the correct sign/zero extension. U16/U8 zero-extend
822+
// into the wider signed SQL type (INTEGER/SMALLINT) they map to.
823+
case I32 -> ((IntArray) array).getInt(r);
816824
case I16, U16 -> ((ShortArray) array).getInt(r);
817825
case I8, U8 -> ((ByteArray) array).getInt(r);
818826
default -> throw new IllegalStateException("unsupported ptype: " + p.ptype());
@@ -823,6 +831,20 @@ private static Object value(Object array, DType type, long r) {
823831
};
824832
}
825833

834+
/// Guards a U64 value being surfaced to Calcite's signed `BIGINT`: values with the high bit set
835+
/// (`>= 2^63`) have no lossless signed-long representation, so this throws rather than let a
836+
/// negative reach SQL. Vortex has no wider unsigned SQL type to widen into, so loud beats wrong.
837+
///
838+
/// @param raw the raw U64 bits read from the column
839+
/// @return `raw` unchanged when it fits the non-negative signed-long range
840+
private static long unsignedBigint(long raw) {
841+
if (raw < 0) {
842+
throw new VortexException("U64 value " + Long.toUnsignedString(raw)
843+
+ " exceeds the signed BIGINT range Calcite maps U64 to");
844+
}
845+
return raw;
846+
}
847+
826848
/// Translates the Calcite predicates into a zone-map [RowFilter], keeping only the comparisons
827849
/// we can prune on (`=`, `<>`, `<`, `<=`, `>`, `>=` between a column and a literal, plus `AND`).
828850
/// 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) {
966988
};
967989
}
968990

991+
/// Maps a Vortex dtype to a Calcite SQL type. Unsigned integers map to the next wider signed
992+
/// SQL type so the declared type can hold their full range (`U8`->`SMALLINT`, `U16`->`INTEGER`,
993+
/// `U32`->`BIGINT`); `U64` is the exception — no wider SQL integer exists, so it maps to
994+
/// `BIGINT` and [#value(Object, DType, long)] guards the high-half values that cannot fit.
995+
///
996+
/// @param factory the Calcite type factory
997+
/// @param type the Vortex column dtype
998+
/// @return the nullable-adjusted SQL type
969999
private static RelDataType toSqlType(RelDataTypeFactory factory, DType type) {
9701000
RelDataType sql = switch (type) {
9711001
case DType.Primitive p -> switch (p.ptype()) {
9721002
case F64 -> factory.createSqlType(SqlTypeName.DOUBLE);
9731003
case F32 -> factory.createSqlType(SqlTypeName.REAL);
974-
case I64, U64 -> factory.createSqlType(SqlTypeName.BIGINT);
975-
case I32, U32 -> factory.createSqlType(SqlTypeName.INTEGER);
976-
case I16, U16 -> factory.createSqlType(SqlTypeName.SMALLINT);
977-
case I8, U8 -> factory.createSqlType(SqlTypeName.TINYINT);
1004+
case I64, U64, U32 -> factory.createSqlType(SqlTypeName.BIGINT);
1005+
case I32, U16 -> factory.createSqlType(SqlTypeName.INTEGER);
1006+
case I16, U8 -> factory.createSqlType(SqlTypeName.SMALLINT);
1007+
case I8 -> factory.createSqlType(SqlTypeName.TINYINT);
9781008
default -> throw new IllegalStateException("unsupported ptype: " + p.ptype());
9791009
};
9801010
case DType.Utf8 _ -> factory.createSqlType(SqlTypeName.VARCHAR);
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
package io.github.dfa1.vortex.calcite;
2+
3+
import io.github.dfa1.vortex.core.error.VortexException;
4+
import io.github.dfa1.vortex.core.model.ColumnName;
5+
import io.github.dfa1.vortex.core.model.DType;
6+
import io.github.dfa1.vortex.reader.ReadRegistry;
7+
import io.github.dfa1.vortex.reader.VortexReader;
8+
import io.github.dfa1.vortex.writer.VortexWriter;
9+
import io.github.dfa1.vortex.writer.WriteOptions;
10+
11+
import org.apache.calcite.linq4j.Enumerator;
12+
import org.junit.jupiter.api.Test;
13+
import org.junit.jupiter.api.io.TempDir;
14+
15+
import java.nio.channels.FileChannel;
16+
import java.nio.file.Path;
17+
import java.nio.file.StandardOpenOption;
18+
import java.util.ArrayList;
19+
import java.util.List;
20+
import java.util.Map;
21+
22+
import static org.assertj.core.api.Assertions.assertThat;
23+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
24+
25+
/// Unsigned high-half handling in the Calcite adapter (#216): U32 widens losslessly into `BIGINT`
26+
/// and U64 — mapped to signed `BIGINT` because no wider SQL integer exists — fails loud rather than
27+
/// surfacing a two's-complement negative to SQL. The canonical motivating shape is an unsigned
28+
/// integer column (like uci-wine's magnesium); here each column carries a boundary high-half value.
29+
class UnsignedColumnTest {
30+
31+
// A U32 value beyond signed INTEGER (0xB2D05E00): getInt reads it as a negative int.
32+
private static final long U32_HIGH = 3_000_000_000L;
33+
// A U64 value with the high bit set (2^63): getLong reads it as Long.MIN_VALUE.
34+
private static final long U64_HIGH = Long.MIN_VALUE;
35+
36+
private static final DType.Struct SCHEMA = DType.structBuilder()
37+
.field("u32", DType.U32)
38+
.field("u64", DType.U64)
39+
.build();
40+
41+
@TempDir
42+
Path tmp;
43+
44+
@Test
45+
void scan_widensU32ToLongAndKeepsInRangeU64() throws Exception {
46+
// Given — a row whose U32 exceeds signed INTEGER and whose U64 stays in the low half
47+
Path file = write("in-range.vortex", WriteOptions.defaults(), (int) U32_HIGH, 4000L);
48+
49+
// When — a full scan materializes the row through VortexTable.value
50+
List<Object[]> rows = drain(new VortexTable(file));
51+
52+
// Then — U32 widened to a positive Long (not a negative int), U64 kept as its Long value
53+
assertThat(rows).hasSize(1);
54+
assertThat(rows.getFirst()[0]).isEqualTo(U32_HIGH);
55+
assertThat(rows.getFirst()[1]).isEqualTo(4000L);
56+
}
57+
58+
@Test
59+
void scan_u64HighHalf_failsLoudRatherThanSurfaceNegative() throws Exception {
60+
// Given — a U64 value with no lossless signed-BIGINT representation
61+
Path file = write("u64-high.vortex", WriteOptions.defaults(), 0, U64_HIGH);
62+
VortexTable table = new VortexTable(file);
63+
64+
// When / Then — materialization throws instead of yielding a negative BIGINT
65+
assertThatThrownBy(() -> drain(table))
66+
.isInstanceOf(VortexException.class)
67+
.hasMessageContaining("U64 value")
68+
.hasMessageContaining("BIGINT");
69+
}
70+
71+
@Test
72+
void sum_u32HighHalf_widensExactlyOnFullScan() throws Exception {
73+
// Given — zone maps off forces the streaming scanSum path (no per-zone SUM to fold)
74+
Path file = write("sum-u32.vortex", noZoneMaps(), (int) U32_HIGH, 1L);
75+
76+
// When
77+
try (VortexReader reader = VortexReader.open(file, registry())) {
78+
VortexAggregates.Summary result = VortexAggregates.of(reader, "u32");
79+
80+
// Then — the U32 element widens via toUnsignedLong, not sign-extension
81+
assertThat(result.sumSource()).isEqualTo(VortexAggregates.Source.FULL_SCAN);
82+
assertThat(result.sum()).isInstanceOf(Long.class).isEqualTo(U32_HIGH);
83+
}
84+
}
85+
86+
@Test
87+
void sum_u64HighHalf_failsLoudOnFullScan() throws Exception {
88+
// Given — a U64 element the signed accumulator cannot represent, zone maps off
89+
Path file = write("sum-u64.vortex", noZoneMaps(), 0, U64_HIGH);
90+
91+
// When / Then — scanSum throws rather than corrupt the sum with a negative addend
92+
try (VortexReader reader = VortexReader.open(file, registry())) {
93+
assertThatThrownBy(() -> VortexAggregates.of(reader, "u64"))
94+
.isInstanceOf(VortexException.class)
95+
.hasMessageContaining("U64 value");
96+
}
97+
}
98+
99+
private Path write(String name, WriteOptions opts, int u32, long u64) throws Exception {
100+
Path file = tmp.resolve(name);
101+
try (FileChannel ch = FileChannel.open(file, StandardOpenOption.CREATE, StandardOpenOption.WRITE);
102+
VortexWriter writer = VortexWriter.create(ch, SCHEMA, opts)) {
103+
writer.writeChunk(Map.of(
104+
ColumnName.of("u32"), new int[]{u32},
105+
ColumnName.of("u64"), new long[]{u64}));
106+
}
107+
return file;
108+
}
109+
110+
private static WriteOptions noZoneMaps() {
111+
// Same shape as the adapter coverage test's zone-maps-off options: the second flag disables
112+
// zone maps so no per-zone SUM exists and VortexAggregates falls back to scanSum.
113+
return new WriteOptions(65_536, false, 0.90, 0, true, false);
114+
}
115+
116+
private static ReadRegistry registry() {
117+
return ReadRegistry.builder().registerServiceLoaded().build();
118+
}
119+
120+
private static List<Object[]> drain(VortexTable table) {
121+
List<Object[]> rows = new ArrayList<>();
122+
Enumerator<Object[]> en = table.scan(null, List.of(), null).enumerator();
123+
try {
124+
while (en.moveNext()) {
125+
rows.add(en.current());
126+
}
127+
} finally {
128+
en.close();
129+
}
130+
return rows;
131+
}
132+
}

calcite/src/test/java/io/github/dfa1/vortex/calcite/VortexAdapterCoverageTest.java

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,13 @@ void getRowType_mapsEveryColumnToItsSqlType() {
8282
// Given / When
8383
RelDataType rowType = new VortexTable(file).getRowType(new JavaTypeFactoryImpl());
8484

85-
// Then — one SqlTypeName per logical type
85+
// Then — one SqlTypeName per logical type. Unsigned integers map to the next wider signed
86+
// SQL type so the declared type holds their full range (#216); U64 is the exception (no
87+
// wider SQL integer) and stays BIGINT with a value-side overflow guard.
8688
Map<String, SqlTypeName> expected = Map.ofEntries(
87-
Map.entry("i8", SqlTypeName.TINYINT), Map.entry("u8", SqlTypeName.TINYINT),
88-
Map.entry("i16", SqlTypeName.SMALLINT), Map.entry("u16", SqlTypeName.SMALLINT),
89-
Map.entry("i32", SqlTypeName.INTEGER), Map.entry("u32", SqlTypeName.INTEGER),
89+
Map.entry("i8", SqlTypeName.TINYINT), Map.entry("u8", SqlTypeName.SMALLINT),
90+
Map.entry("i16", SqlTypeName.SMALLINT), Map.entry("u16", SqlTypeName.INTEGER),
91+
Map.entry("i32", SqlTypeName.INTEGER), Map.entry("u32", SqlTypeName.BIGINT),
9092
Map.entry("i64", SqlTypeName.BIGINT), Map.entry("u64", SqlTypeName.BIGINT),
9193
Map.entry("f32", SqlTypeName.REAL), Map.entry("f64", SqlTypeName.DOUBLE),
9294
Map.entry("s", SqlTypeName.VARCHAR), Map.entry("b", SqlTypeName.BOOLEAN));

0 commit comments

Comments
 (0)