Repository navigation
fix: decimal dtype bounds and FoR unsigned reference, as Rust - #571
Merged
Merged
Conversation
Rust's DecimalDType::try_new wants a precision in 1..=76 and a scale no greater than it. Only the FlatBuffer dtype parser checked that, so DType.decimal(80, 0) or (10, 12) wrote a file that vortex-jni and our own reader both refuse, and a vortex.variant decimal (parsed from proto) was never checked, with its u32/i32 fields truncated by a (byte) cast where Rust's try_into errors (300 became the valid precision 44). The rule moves into DType.Decimal itself, as DType.Map already does for its key: the compact constructor checks the byte components and Decimal.of checks wide values before narrowing. It throws VortexException, because the file parsers construct through it. The parser's own copy is gone. Breaking: such a dtype used to be accepted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVGQ5h5Lk6Zn4JnkxF1K
Rust's FoRData::encode subtracts Stat::Min, an unsigned minimum for a u64 column. computeRef took a signed one for every ptype, so for u64 values on both sides of 2^63 it picked 2^63 (Long.MIN_VALUE) over a smaller value below it, leaving that value a full-width residual where Rust's are a few bits. The cascade's estimate already used the unsigned min (ArrayStats), so estimate and encode disagreed too. The files were readable (decode is a wrapping add); this is compression parity. Widened U8..U32 values are zero-extended, so the U64 order serves every unsigned width. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVGQ5h5Lk6Zn4JnkxF1K
…ni targets The differential target found every writer interop bug of the fuzz round, but it wrote only ints, floats, bools and utf8. Decimals at each physical width Rust picks (i32, i64, i128) and arbitrary-byte binary columns now join them, with the same value shapes. 116,767 inputs over five minutes: Java and vortex-jni agree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVGQ5h5Lk6Zn4JnkxF1K
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.
Follow-up review of the fuzz round (#493), looking for more bugs of the kinds it found.
DType.decimal(80, 0)or(10, 12)wrote files that vortex-jni and our reader refuse; avortex.variantdecimal was never checked, and its protou32/i32fields were truncated by a(byte)cast (300 → a valid 44) where Rust errors. The rule (Rust'sDecimalDType::try_new) now lives inDType.Decimal, likeDType.Map's key rule. Breaking.u64values spanning 2^63, the signed min left values at full width where Rust'sStat::Minpacks them in a few bits. Compression parity only; files were readable.Checked:
./mvnw verify: green.🤖 Generated with Claude Code
https://claude.ai/code/session_01WtuVGQ5h5Lk6Zn4JnkxF1K