Skip to content

fix: preserve sliced patch offsets during deserialization - #11168

Merged
robert3005 merged 3 commits into
developfrom
db/patches-owc-repro
Nov 3, 2026
Merged

robert3005 merged 3 commits into
developfrom
db/patches-owc-repro

Conversation

@blaginin

@blaginin blaginin commented Nov 3, 2026

Copy link
Copy Markdown
Member

Preserve and validate sliced patch offsets when deserializing BitPacked and ALP arrays to prevent incorrect values after serialization round trips.

blaginin and others added 3 commits November 3, 2026 17:18
Patches::to_metadata writes offset_within_chunk, but every deserializer
rebuilds the patches with Patches::new, which resets it to 0. After a
round trip of a BitPacked array that was sliced mid-chunk while encoded,
chunked patch lookups (scalar_at, Patches::slice, take) miss the first
patches of every chunk after the first and return the packed placeholder.

The test slices an 11-bit array with a patch every 7th row at 500..4096,
round-trips it through BitPackedPlugin, and reads row 529 (original row
1029, the first patch of the second chunk): it gets 904 instead of 5000.

Co-Authored-By: Claude Mythos 5.1 <[email protected]>
Signed-off-by: blaginin <[email protected]>
Restore offset_within_chunk from patch metadata in BitPacked v1/v2 and ALP, retaining the zero default for older metadata. Cover sliced round trips, patch lookups, take, repeated slices, and legacy metadata.

Signed-off-by: blaginin <[email protected]>
Check restored offsets against the sliced row count and host-resident first-chunk patch indices. Cover malformed offsets and valid short single-chunk slices. Remove the unused ALP import and apply the CI-pinned formatter.

Signed-off-by: blaginin <[email protected]>
@blaginin blaginin added the changelog/chore A trivial change label Nov 3, 2026
@blaginin
blaginin requested a review from myrrc November 3, 2026 18:09
@blaginin blaginin added the changelog/fix A bug fix label Nov 3, 2026
@blaginin
blaginin marked this pull request as ready for review November 3, 2026 18:09
@blaginin blaginin removed the changelog/chore A trivial change label Nov 3, 2026
@blaginin blaginin self-assigned this Nov 3, 2026
@codspeed

codspeed Bot commented Nov 3, 2026

Copy link
Copy Markdown

Merging this PR will regress 1 benchmark

1 warning

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

⚡ 4 improved benchmarks
❌ 1 regressed benchmark
✅ 1877 untouched benchmarks
⏩ 469 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Walltime lt_i64_nullable_avx512 7.3 µs 8.2 µs -10.65%
⚡ CPU Simulation missing_stats[16] 85.3 µs 67.6 µs +26.26%
⚡ CPU Simulation filter_powerlaw_by_random[10000] 2.3 µs 1.9 µs +23.56%
⚡ CPU Simulation take_filter_primitive_small_uncached_random_mask_random_indices[4096, 10] 24.6 µs 21.8 µs +12.82%
⚡ Walltime scalar_subtract_neon 11.5 µs 10.2 µs +12.24%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing db/patches-owc-repro (1a797f5) with develop (ce6ccb8)

Open in CodSpeed

Footnotes

  1. 469 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
robert3005 merged commit 8bada73 into develop Nov 3, 2026
222 of 226 checks passed
@robert3005
robert3005 deleted the db/patches-owc-repro branch November 3, 2026 19:45
connortsui20 added a commit that referenced this pull request Nov 3, 2026
## Summary

Follow-up to #11110. Keep the remaining noisy cases out of automatic
CodSpeed runs while preserving them for local benchmarks.

The Python scalar exclusions in #11110 never took effect because the
runner exports simulation as `instrumentation`, while the fixture
checked for `simulation`. Use the plugin's resolved mode so these cases
are actually skipped.

## Changes

- Allow a trailing comma in `#[divan::bench(...)]` when the CPU-feature
macro appends its options. The multiline bitpacked take definition
covers this case in CI builds.
- Share the resolved simulation check between the scalar and filter
fixtures. Also skip the 2 integer one-shot filtered scans that still
report system calls under simulation.
- Keep AVX2 bitpacked take with 80 selections local after batching still
produced a regression alert. Retain the 64-selection case and both
AVX-512 cases in CI.
- Keep AVX-512 nullable integer comparison local because its minimum
changes by more than 10% while its median stays stable.
- Keep the small random power-law boolean filter and the large
correlated power-law boolean filter local under simulation. The former
has repeated unexplained changes, and the latter repeatedly makes system
calls that simulation cannot measure.

<details>
<summary>Evidence from recent PR reports</summary>

Reviewed 40 CodSpeed report comments updated between November 2 at 19:28
UTC and November 3 at 19:28 UTC, including their comparison revisions
and the affected measurement modes.

| Case | Evidence |
| --- | --- |
| Python integer scalar scans |
[#11154](#11154 (comment))
still reports the repeated scan's 10 system calls. [A develop run after
#11110](https://github.com/vortex-data/vortex/actions/runs/41677647530/job/126062463160)
executes both integer scalar cases instead of skipping them. The [runner
exports the legacy `instrumentation`
value](https://github.com/CodSpeedHQ/codspeed/blob/main/crates/runner-shared/src/measurement_mode.rs),
which pytest-codspeed 5.0.3 normalizes to simulation. |
| Python integer filtered scans |
[#11130](#11130 (comment))
reports 30 system calls totaling 14.3 ms in `test_scan_filter[int]`.
[#11138](#11138 (comment))
reports 30 calls totaling 16.3 ms in
`test_scan_filter_to_arrow_array[int]`. |
| AVX2 `threshold[80]` |
[#11004](#11004 (comment))
compares `f7b5b1d864` with `e59b236d7b`: 2.183 to 2.531 µs, reported as
a 13.75% regression. Both use 1000 samples of 16 iterations. The head
median is 2.556 µs, so this is not just a single outlying minimum. The
PR does not change the fastlanes code or benchmark. |
| AVX-512 `lt_i64_nullable` |
[#11168](#11168 (comment))
reports 7.342 to 8.217 µs. Several unrelated CUDA PRs report the
opposite direction, including
[#10872](#10872 (comment)),
at 8.208 to 7.348 µs. Their head medians remain around 8.3 to 8.5 µs. |
| Boolean `filter_powerlaw_by_random[10000]` |
[#11167](#11167 (comment))
reports a 16.2% regression,
[#11168](#11168 (comment))
a 23.56% improvement, and
[#11169](#11169 (comment))
a 13.06% regression. The cause is unknown. |
| Boolean `filter_powerlaw_by_correlated_runs[100000]` |
[#10871](#10871 (comment))
and
[#11165](#11165 (comment))
each report 2 system calls totaling about 44 µs. |

The NEON chunked-take alerts on #11090 are on a PR that changes
execution substantially, so those cases remain enabled. Other dictionary
simulation alerts need more attribution before excluding them.

</details>

CI passes formatting, Python lint and script tests, Rust lint,
default-feature and all-feature Rust builds, all 3 CPU walltime
benchmark jobs, and Python benchmarks (36 passed, 4 intentionally
skipped). The previously failed simulation shard also passes. Some
remaining CI jobs and CodSpeed performance analysis are still pending.

No local checks were run. The pre-commit and pre-push hooks were
bypassed as requested.

---------

Signed-off-by: Connor Tsui <[email protected]>
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