Repository navigation
Fix dictionary code width for nullable primitive values - #11165
Conversation
…alue `primitive_dict_builder` caps the dictionary length at the value type's cardinality minus one (`u8::MAX`) and picks `u8` codes for it, but a nullable column that holds all 256 values also needs a code for null. The 257th code does not fit and `encode_null` panics with "256 has to fit into u8". The same happens for `u16`/`i16` at 65 536 values. Co-Authored-By: Claude Mythos 5.1 <[email protected]> Signed-off-by: blaginin <[email protected]>
Reserve a code for null when bounding the primitive value domain, and compare the resulting maximum code with the zero-based dictionary length limit. Saturate the 64-bit bound to avoid overflow. Expand the regression to signed and unsigned 8-bit and 16-bit values, with null first or last, and cover code-width boundaries and full decoded values. Signed-off-by: blaginin <[email protected]>
Signed-off-by: blaginin <[email protected]>
Merging this PR will improve performance by 16.26%2 warnings
Performance Changes
Tip Curious why performance improved? Comment Comparing Footnotes
|
## 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]>
Account for the extra null entry when choosing primitive dictionary code widths, preventing overflow at full cardinality.