fix: unsigned dtype handling in TUI, filter predicates, and Calcite (#216)#220
Merged
Conversation
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #216, the follow-up to #208's CSV fix — the same dtype-blindness in the remaining consumers, two of them wrong-query-results class:
filterpredicates compared raw signed getters:magnesium >= 130on uci-wine silently returned 0 of its 6 matching rows (values 132–162 read as negatives). Now widens U8/U16/U32 losslessly and compares U64 withLong.compareUnsigned— the real-file spot check returns exactly the 6 rows pyarrow confirms.VortexException) rather than surfacing a two's-complement negative for values ≥ 2^63, both on read (VortexTable.value) and inscanSum(branch-split on the loop-invariant unsigned flag, keeping the signed body vectorizable per the hot-loop rule).toUnsignedString.+9 tests (cli 190, calcite 76);
./mvnw verify -DskipITs -pl csv,cli,calcite -amgreen after rebase onto main with all six sibling fixes.Closes #216
🤖 Generated with Claude Code