Repository navigation
Fix Dict rewrites over unreferenced values - #11169
Merged
Merged
Conversation
Slicing, filtering or taking a dictionary keeps the whole values array and only selects codes, so values that no remaining row references stay in the dictionary. `CastReduce for Dict` and `DictionaryScalarFnCodesPullUpRule` then apply a fallible function to every value, and a narrowing cast or a division fails on a value that row-wise evaluation would never visit. Both tests build valid arrays with the public constructors and fail today: the cast reports "value exceeds target range" for rows [1, 1, 1], and the division reports "division by zero" for [8, 8, 8] / [2, 2, 2]. Co-Authored-By: Claude Mythos 5.1 <[email protected]> Signed-off-by: blaginin <[email protected]>
Keep value-changing casts on dictionary rows unless all values are known to be referenced. Preserve outer-nullability rewrites with the existing validity guard. Apply the same fallibility guard to shared-code scalar-function rewrites and cover safe rewrites and errors on referenced values. Signed-off-by: blaginin <[email protected]>
Merging this PR will not alter performance2 warnings
Comparing Footnotes
|
robert3005
approved these changes
Nov 3, 2026
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]>
Signed-off-by: blaginin <[email protected]>
Share the primitive cast kernel’s lossless conversion check with the dictionary rewrite so widening preserves the codes even when value coverage is unknown. Exercise the rewrites with the session optimizer now that constructors are lazy. Signed-off-by: blaginin <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prevent dictionary cast and scalar-function rewrites from failing on unreferenced values, with regression coverage for both cases.