Skip to content

Fix patch_chunk index OOB when slicing ALP arrays mid-chunk#7354

Merged
robert3005 merged 2 commits intovortex-data:developfrom
abnobdoss:fix/patch-chunk-end-idx-clamp
Apr 9, 2026
Merged

Fix patch_chunk index OOB when slicing ALP arrays mid-chunk#7354
robert3005 merged 2 commits intovortex-data:developfrom
abnobdoss:fix/patch-chunk-end-idx-clamp

Conversation

@abnobdoss
Copy link
Copy Markdown

Summary

patch_chunk panics with index-out-of-bounds when decompressing sliced ALP arrays with patches. The slice boundary must fall mid-chunk (non-1024-aligned) to trigger it.

Root cause: Patches::slice() slices chunk_offsets at chunk granularity but indices/values at element granularity. When a slice ends mid-chunk, patches_end_idx can exceed patches_indices.len(). The start index already used saturating_sub — the end index didn't.

Fix: .saturating_sub(offset_within_chunk).min(patches_indices.len()) on the end index computation in patch_chunk, search_index_chunked, and search_index_chunked_batch.

AI disclosure: Root cause analysis, fix implementation, and test writing were done with Claude Code under my direction and review. I identified the bug during development benchmarking at 100M rows and verified the fix against my workload.

Testing

  • Unit test calling patch_chunk with inputs that OOB without the fix.
  • Integration test: ALP encode 3-chunk array with patches, slice mid-chunk, decode. Tests both start-aligned and mid-chunk-start variants.
  • All existing tests pass (2538 vortex-array, 167 vortex-alp).

Signed-off-by: Abanoub Doss <abanoub.doss@gmail.com>
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 9, 2026

Merging this PR will degrade performance by 23.81%

⚡ 2 improved benchmarks
❌ 24 regressed benchmarks
✅ 1096 untouched benchmarks
⏩ 1530 skipped benchmarks1

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

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation take_search[(0.005, 0.05)] 131.5 µs 168.1 µs -21.74%
Simulation take_search[(0.01, 0.05)] 142.6 µs 179.1 µs -20.4%
Simulation take_search[(0.01, 0.5)] 1.3 ms 1.6 ms -22.22%
Simulation take_search[(0.01, 0.1)] 267.8 µs 340.8 µs -21.41%
Simulation take_search[(0.1, 0.05)] 212.4 µs 249 µs -14.69%
Simulation take_search[(0.005, 0.1)] 247.1 µs 320 µs -22.8%
Simulation take_search[(0.1, 0.5)] 1.8 ms 2.2 ms -16.94%
Simulation take_search[(0.005, 0.5)] 1.2 ms 1.5 ms -23.68%
Simulation take_search[(0.005, 1.0)] 2.3 ms 3.1 ms -23.81%
Simulation take_search[(0.01, 1.0)] 2.5 ms 3.3 ms -22.34%
Simulation take_search_chunked[(0.005, 0.5)] 1.5 ms 1.9 ms -18.63%
Simulation take_search_chunked[(0.005, 0.1)] 314.5 µs 383.7 µs -18.02%
Simulation take_search[(0.1, 0.1)] 386.5 µs 459.4 µs -15.89%
Simulation take_search[(0.1, 1.0)] 3.5 ms 4.3 ms -17.1%
Simulation take_search_chunked[(0.005, 0.05)] 165.9 µs 200.5 µs -17.26%
Simulation take_search_chunked[(0.1, 0.05)] 244.7 µs 279.2 µs -12.38%
Simulation take_search_chunked[(0.01, 0.5)] 1.6 ms 2 ms -17.51%
Simulation take_search_chunked[(0.1, 0.1)] 450 µs 519.1 µs -13.31%
Simulation take_search_chunked[(0.1, 1.0)] 4.2 ms 4.9 ms -14.21%
Simulation take_search_chunked[(0.005, 1.0)] 3 ms 3.7 ms -18.72%
... ... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.


Comparing abnobdoss:fix/patch-chunk-end-idx-clamp (f62f117) with develop (a30de02)

Open in CodSpeed

Footnotes

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

@joseph-isaacs joseph-isaacs requested a review from 0ax1 April 9, 2026 10:18
Copy link
Copy Markdown
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.

offset_within_chunk has to always be used with saturating sub, we don't know where exactly the indicies fall in the chunk. I thought that this wasn't necessary for subsequent chunks after the current chunk but we don't know if chunks have any patches at all

Signed-off-by: Abanoub Doss <abanoub.doss@gmail.com>
@robert3005 robert3005 added the changelog/fix A bug fix label Apr 9, 2026
@robert3005 robert3005 merged commit 72d39fb into vortex-data:develop Apr 9, 2026
63 of 65 checks passed
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.

2 participants