Skip to content

Fix Validity::mask_eq semantics for mixed variants#8334

Open
joseph-isaacs wants to merge 1 commit into
claude/cool-bardeen-l8jlsy-1-definitely-no-nullsfrom
claude/cool-bardeen-l8jlsy-2-mask-eq
Open

Fix Validity::mask_eq semantics for mixed variants#8334
joseph-isaacs wants to merge 1 commit into
claude/cool-bardeen-l8jlsy-1-definitely-no-nullsfrom
claude/cool-bardeen-l8jlsy-2-mask-eq

Conversation

@joseph-isaacs

Copy link
Copy Markdown
Contributor

Summary

PR 2 of a 4-PR stack (stacked on #8333) preparing Validity for lazy validity arrays.

mask_eq previously returned false for any mixed-variant pairing without executing — e.g. a Validity::Array that resolves to all-true compared against Validity::AllValid. With lazy validity arrays, unresolved Array variants frequently hold constant masks, making this silently wrong rather than merely conservative.

  • mask_eq now takes the validity length and compares logical masks: constant variants keep their no-execute fast paths, and any pairing involving an Array executes both sides via execute_mask and compares the resulting Masks (whose PartialEq is already representation-independent).
  • NonNullable and AllValid now compare equal, since their logical masks are identical.
  • Adds 9 rstest cases covering mixed constant pairings and arrays resolving to constant masks.

All existing callers (tests/asserts in vortex, datetime-parts, pco, zstd, builders, masked) are updated to pass the length.

Checks

  • cargo nextest run -p vortex-array (2962 passed, incl. new mask_eq_mixed_variants cases)
  • cargo nextest run -p vortex-datetime-parts -p vortex-pco -p vortex-zstd (119 passed)
  • cargo build --tests -p vortex, cargo clippy -p vortex-array --all-targets, cargo +nightly fmt --all

https://claude.ai/code/session_01VPQ7dfZtijfrsjAipwXvEj


Generated by Claude Code

mask_eq previously returned false for any mixed-variant pairing without
executing, e.g. comparing a Validity::Array that resolves to all-true
against Validity::AllValid. As validity arrays become lazy, unresolved
Array variants frequently hold constant masks, making this silently
wrong rather than merely conservative.

mask_eq now takes the validity length and compares logical masks:
constant variants keep their no-execute fast paths, and any pairing
involving an Array executes both sides via execute_mask and compares
the resulting Masks. NonNullable and AllValid now compare equal, since
their logical masks are identical.

https://claude.ai/code/session_01VPQ7dfZtijfrsjAipwXvEj
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
@codspeed-hq

codspeed-hq Bot commented Jun 10, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 18.29%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

❌ 5 regressed benchmarks
✅ 1521 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation chunked_bool_canonical_into[(1000, 10)] 31.8 µs 47 µs -32.34%
Simulation chunked_varbinview_canonical_into[(1000, 10)] 161.8 µs 198.6 µs -18.5%
Simulation chunked_varbinview_into_canonical[(1000, 10)] 177.2 µs 213.2 µs -16.85%
Simulation chunked_varbinview_canonical_into[(100, 100)] 274.5 µs 308.9 µs -11.15%
Simulation bitwise_not_vortex_buffer_mut[128] 246.1 ns 275.3 ns -10.6%

Tip

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


Comparing claude/cool-bardeen-l8jlsy-2-mask-eq (4813bec) with claude/cool-bardeen-l8jlsy-1-definitely-no-nulls (4906359)

Open in CodSpeed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/break A breaking API change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant