Skip to content

fix sum decimal array overflow behavior with scalars#6431

Merged
joseph-isaacs merged 2 commits intodevelopfrom
ct/fix-decimal-overflow-scalar
Feb 11, 2026
Merged

fix sum decimal array overflow behavior with scalars#6431
joseph-isaacs merged 2 commits intodevelopfrom
ct/fix-decimal-overflow-scalar

Conversation

@connortsui20
Copy link
Contributor

@connortsui20 connortsui20 commented Feb 11, 2026

Fixes #6429

Also refactors the code a bit so that we dont need a clippy allow complexity

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20 connortsui20 added the changelog/fix A bug fix label Feb 11, 2026
@connortsui20 connortsui20 changed the title fix sum decimal array overflow fix sum decimal array overflow behavior with scalars Feb 11, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Feb 11, 2026

Merging this PR will degrade performance by 10.22%

⚡ 5 improved benchmarks
❌ 2 regressed benchmarks
✅ 1128 untouched benchmarks
⏩ 1268 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation encode_varbin[(1000, 2)] 203.4 µs 226.5 µs -10.22%
Simulation encode_varbin[(1000, 4)] 205.3 µs 228.5 µs -10.14%
Simulation take_map[(0.05, 0.1)] 792 µs 603 µs +31.34%
Simulation take_map[(0.1, 0.1)] 1,118.8 µs 921.3 µs +21.44%
Simulation take_map[(0.05, 1.0)] 4.1 ms 3.1 ms +33.35%
Simulation take_map[(0.1, 1.0)] 4.7 ms 3.5 ms +34.59%
Simulation take_map[(0.1, 0.5)] 2.8 ms 2.1 ms +34.07%

Comparing ct/fix-decimal-overflow-scalar (838d354) with develop (aa55054)2

Open in CodSpeed

Footnotes

  1. 1268 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on develop (92156f5) during the generation of this report, so aa55054 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@robert3005
Copy link
Contributor

What was the bug here? That we returned overflow value even though it didn't fit into the declared dtype?

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@joseph-isaacs
Copy link
Contributor

overflow the precision but not the storage type?

@joseph-isaacs joseph-isaacs enabled auto-merge (squash) February 11, 2026 16:35
@connortsui20
Copy link
Contributor Author

That we returned overflow value even though it didn't fit into the declared dtype?

Yep, so the summation of the decimal array might not have fit the return dtype (even if it did not overflow the storage value). The regression test included in this change does something similar to what the fuzzer found.

@connortsui20
Copy link
Contributor Author

overflow the precision but not the storage type?

Previously it only checked overflow on the storage type (in the sum_decimal and sum_decimal_with_validity functions with CheckedAdd::checked_add), now it does both

@joseph-isaacs joseph-isaacs merged commit 35ea593 into develop Feb 11, 2026
48 of 50 checks passed
@joseph-isaacs joseph-isaacs deleted the ct/fix-decimal-overflow-scalar branch February 11, 2026 16:44
@robert3005
Copy link
Contributor

Thanks, I should have read the test before asking my question

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fuzzing Crash: VortexError in file_io

3 participants