Skip to content

Fix bitwise_unary_op#6940

Merged
connortsui20 merged 1 commit intodevelopfrom
ct/fix-bit-unary
Mar 13, 2026
Merged

Fix bitwise_unary_op#6940
connortsui20 merged 1 commit intodevelopfrom
ct/fix-bit-unary

Conversation

@connortsui20
Copy link
Contributor

Summary

Closes: #6895 (which might seem completely unrelated but is relevant because of an incorrect mask)

Fixes a bug in bitwise_unary_op where the it used Arrow's UnalignedBitChunk iterator, which for buffers larger than 16 bytes (128 bits) uses align_to::<u64>() and introduces lead padding zeros when the byte pointer isn't u64-aligned. After applying the operation (e.g. NOT), those padding bits were written into a fresh, aligned output buffer at real data positions, corrupting the first padding bits of the result

This change just delegates to the correct mut implementation which is likely also faster.

Testing

Adds a simple regression test.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20 connortsui20 added the changelog/fix A bug fix label Mar 13, 2026
@robert3005
Copy link
Contributor

Ah, that's why you can't just change this. I made this change in #6880 which means that binary op needs adjustment as well. I don't see how the mut op would be faster though

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 13, 2026

Merging this PR will degrade performance by 12.27%

⚡ 1 improved benchmark
❌ 1 regressed benchmark
✅ 1007 untouched benchmarks
⏩ 1515 skipped benchmarks1

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

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation binary_search_std 582.8 ns 524.4 ns +11.12%
Simulation true_count_vortex_buffer[128] 1 µs 1.2 µs -12.27%

Comparing ct/fix-bit-unary (e0daf70) with develop (18bef2b)

Open in CodSpeed

Footnotes

  1. 1515 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.

@robert3005
Copy link
Contributor

align_to is not the problem here though? It's that the padding is wrong so offset and length are wrong

@connortsui20
Copy link
Contributor Author

I thought that align to caused the padding to be wrong in this case.

Anyways I don't see much reason to not just delegate to the _mut version, the regression on codspeed is likely because it was just incorrect before.

@robert3005
Copy link
Contributor

align_to just splits the buffer (it's a const function). But the padding is clearly not preserved for any buffer in 17-32 range potentially. Anyway I agree with your fix. We should make sure to fix the binary op as well since we're here

@connortsui20
Copy link
Contributor Author

what do you mean by fix the binary op?

Copy link
Contributor

@robert3005 robert3005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can follow up with the fix to binary op

@connortsui20 connortsui20 merged commit 4b7207e into develop Mar 13, 2026
54 of 56 checks passed
@connortsui20 connortsui20 deleted the ct/fix-bit-unary branch March 13, 2026 15:58
@robert3005
Copy link
Contributor

Ok, binary op is not a problem since it's only ever used for aligned arrays (i.e. 8 byte aligned) but we really only need 1 byte alignment

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 array_ops

2 participants