fix: render unsigned integers unsigned in CSV export (#208)#213
Merged
Conversation
cellValue() formatted integer cells by array class alone (Byte/Short/Int/ LongArray), never consulting dtype signedness. High-half values in U8/U16/ U32/U64 columns were printed as their two's-complement negatives — silent value corruption on export. Byte/ShortArray.getInt already zero-extend per their dtype, so those two arms now widen to int and print signed decimal (correct for both signed and unsigned). Int/LongArray have no dtype-aware getter, so a small isUnsigned() helper gates Integer.toUnsignedString / Long.toUnsignedString on the ptype. The helper reads dtype().ptype(), which a MaskedArray's inner values array carries identically, so the nullable recursion stays correct. Real-world evidence: uci-wine `magnesium` (U8?, values 70–162) exported row 34 as -124 instead of 132; palmer-penguins `flipper_length_mm` (U8, 172–231) exported every row negative. Verified the uci-wine export now matches the parquet oracle (132) after the fix. Closes #208 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
Reviewed (vortex-reviewer agent, full diff + tests executed at the PR ref): approve. Correctness verified on all axes — signed I8/I16 output unchanged (dtype-gated zero-extension only), MaskedArray recursion consults the leaf array's ptype, |
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 #208, found by the Raincloud conformance suite (#205):
CsvExporter.cellValueformatted integer cells purely by array class, never consulting dtype signedness, so U8/U16/U32/U64 high-half values printed as two's-complement negatives — silent corruption (uci-winemagnesiumU8 132 exported as -124; penguinsflipper_length_mmall 344 rows negative).ByteArray/ShortArrayarms now go through their dtype-awaregetIntdefaults (zero-extend on U8/U16).IntArray/LongArraygain anisUnsigned(dtype)gate choosingInteger.toUnsignedString/Long.toUnsignedString.-1L→18446744073709551615.Verified on the real corpus: uci-wine
magnesiumrow 34 now exports132matching the Parquet oracle.Survey (follow-up issues to file, not fixed here) — same dtype-blindness elsewhere:
GridRender.java:61-64,InspectorRender.java:51-54,FilterCommand.java:203-206(filter predicates compare U* as signed),VortexTable.java:812-817(Calcite U32/U64 raw),VortexAggregates.java:107,112(U32/U64 sums wrong).Merge notes: independent of #212, either order; trivial CHANGELOG conflict with it. After both merge, the conformance matrix flips
uci-wine → ok(andpenguins → oktogether with #212) in a coordination commit.Closes #208
🤖 Generated with Claude Code