fix(decimal): use minimal byte length for negative powers of two - #3523
fix(decimal): use minimal byte length for negative powers of two#3523vishnuprakaz wants to merge 2 commits into
Conversation
| # The same applies when the unscaled value comes from a Decimal. | ||
| assert bytes_required(Decimal("-1.28")) == 1 | ||
| assert bytes_required(Decimal("-327.68")) == 2 |
There was a problem hiding this comment.
Can we add a positive Decimal just in case? Decimal("1.27")
There was a problem hiding this comment.
Yep! good idea
bytes_required
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
2 participants
Closes #3522
Rationale for this change
bytes_requiredshould return the minimum number of bytes fora value, but it returns one byte too many for negatives like
-128 and -32768.
This matters because the decimal bucket transform hashes those
bytes. The extra byte changes the hash, so PyIceberg can put a
value in a different bucket than Spark/Java. For example,
Decimal("-1.28")goes to bucket 12 in PyIceberg but bucket 13everywhere else.
The fix computes the length from
(value + 1)for negativevalues, which gives the true minimum and matches the Iceberg
spec.
Are these changes tested?
Yes. Added tests for the -128 / -32768 / -8388608 boundary cases
(where the old code was wrong) plus positive/negative controls.
Lint and the decimal/conversion/transform tests pass.
Are there any user-facing changes?
Yes. For decimal values like -1.28, the computed bucket changes
(e.g. 12 → 13) so it now matches other Iceberg engines.