Store Normalized validity on the array instead of in its children - #9202
Open
connortsui20 wants to merge 5 commits into
Open
Store Normalized validity on the array instead of in its children#9202connortsui20 wants to merge 5 commits into
Normalized validity on the array instead of in its children#9202connortsui20 wants to merge 5 commits into
Conversation
connortsui20
marked this pull request as draft
August 5, 2026 16:17
Merging this PR will degrade performance by 5.77%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | cold_misaligned[(64, 256)] |
4.4 ms | 5.3 ms | -17.24% |
| ❌ | Simulation | take[small_m/shuffled/primitive/nonnull/chunks=16384/indices=16] |
1.2 ms | 1.3 ms | -10.33% |
| ⚡ | Simulation | decompress[u64, (1000, 16)] |
72.2 µs | 64 µs | +12.76% |
| 🆕 | Simulation | encode_non_nullable[2] |
N/A | 897.3 µs | N/A |
| 🆕 | Simulation | encode_non_nullable[256] |
N/A | 257.7 µs | N/A |
| 🆕 | Simulation | encode_non_nullable[32] |
N/A | 310.1 µs | N/A |
| 🆕 | Simulation | encode_nullable[2] |
N/A | 1.1 ms | N/A |
| 🆕 | Simulation | encode_nullable[256] |
N/A | 333.4 µs | N/A |
| 🆕 | Simulation | encode_nullable[32] |
N/A | 370.5 µs | N/A |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ct/fix-normalized (91c96d6) with develop (19f771f)
Footnotes
-
85 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. ↩
connortsui20
marked this pull request as ready for review
August 6, 2026 20:57
Member
Author
|
Do we think that it is fine to require both children to be non-nullable and store validity separately? I think it makes the most sense (to dedpulicate validity and make things consistent), especially since |
connortsui20
force-pushed
the
ct/fix-normalized
branch
from
August 7, 2026 18:38
a810e59 to
d1fcb0a
Compare
connortsui20
marked this pull request as draft
August 7, 2026 18:46
connortsui20
force-pushed
the
ct/fix-normalized
branch
2 times, most recently
from
August 7, 2026 19:27
f51af4c to
a71a19a
Compare
connortsui20
added
changelog/break
Closes #9200 `Normalized` derived its validity by `and`ing its two children's, which made the parent's nullability a function of two independently nullable children. That is what every dtype-widening bug in the encoding came from: the decode paths and the read-through operators each had to get from a child's nullability to the parent's, and each got it wrong in a different case. Nulls now live on the array. Both children are non-nullable, `normalize` zeroes them at null positions, and the array carries a `validity` slot whose presence is what makes its dtype nullable. Neither the decode path nor a read-through operator has to widen a child's dtype any more, and `validity()` is a match on a slot rather than a `Binary(And)` array that has to be built and optimized on every call. ## Fixes - Both children nullable plus a non-unit constant norm panicked with `Tried to create an `ExtensionArray` with an incompatible storage array`: the dtype guard on the constant path caught a nullable norms child paired with a non-nullable normalized child, but not the case where both were nullable, and the multiply then widened the FSL elements. The guard is gone along with the state it guarded against. - `L2Norm`'s read-through asserted exact dtype equality against a `norm_dtype` it took from the parent, so it failed on any column whose nullability came from the `normalized` child. It now reattaches the array's null map to the non-nullable norms child. - `try_new` accepted an all-zero row paired with a non-zero stored norm. That decodes to zeros while `L2Norm` reads the stored norm straight back, so the split it promises is lossless was not. The zero-norm rule is now checked in both directions. - `NormalizedScheme::matches` claimed any `AnyTensor` extension, but `compress` gates on a float element ptype. Since the scheme reports `AlwaysUse`, an `i32` tensor column was claimed and then aborted the whole column's compression instead of falling through to another scheme. `matches` now requires a float element ptype. ## Also - `NormalizedMetadata` is removed. It existed only because the parent's unioned nullability could not say which child was nullable; with non-nullable children, the parent dtype and the child count carry everything, so the array serializes no metadata. - The constant-norms identity path now requires a norm of exactly `1.0`. Skipping the multiply for a merely near-unit norm left `scalar_at` -- which routes every row through that path -- answering differently than a bulk decode of the same column. - `validate_l2_normalized_rows_against_norms` is renamed to `validate_normalized_rows`, and no longer takes validity into account: a zeroed null row satisfies both directions of the zero-norm rule on its own. - Fixes "An `Normalized`" in the eight places the rename left it, and restores the alphabetical order of the 2026-04 edition's `added` list. - `benches/normalized.rs` passes the validity to `try_new` rather than wrapping the normalized child in a `MaskedArray`, which keeps the arm names `non_nullable` and `nullable` stable for CodSpeed. ## Checks | Check | Result | | --- | --- | | `cargo test -p vortex-tensor` | 180 passed | | `cargo test -p vortex --features unstable_encodings` | 26 passed | | `cargo test -p vortex-file --features unstable_encodings` | 133 passed | | `cargo test -p vortex-btrblocks -p vortex-compressor` | 93 passed, 1 skipped | | `cargo clippy -p vortex-tensor -p vortex -p vortex-bench --all-targets --features vortex/unstable_encodings` | clean | | `cargo clippy -p vortex-tensor --all-targets --all-features` | clean | | `cargo +nightly fmt --all` | clean | | `cargo test --doc -p vortex-tensor -p vortex` | clean | Each of the four functional fixes was confirmed to reproduce before the change, and each regression test was confirmed to fail when its fix alone is reverted. Not run: workspace-wide `cargo clippy --all-features` (the `vortex-cuda` build script needs to download nvCOMP, which this sandbox cannot reach), Python/Java bindings, docs. Signed-off-by: Connor Tsui <connor@spiraldb.com> Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
`validity_to_child` writes no child for `Validity::AllValid`, so a nullable `Normalized` with no null rows persists only its two data children and the parent dtype is the sole record of the nullability. The constant fast path produces exactly this state for a nullable input whose stored row is not null, and nothing exercised it. Signed-off-by: Connor Tsui <connor@spiraldb.com> Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
`normalized_parts` derived the array dtype with `normalized.dtype().union_nullability(validity.nullability())`, which lets the `normalized` child's own nullability contribute to the parent's. That is the coupling this change set exists to remove, and it is not how the canonical containers work: `Bool`, `Struct`, `List`, `ListView`, and `FixedSizeList` all build their dtype with `validity.nullability()` and ignore the child's. Under the enforced invariant the two are equivalent, since the children are always non-nullable. The difference shows up through `new_unchecked`, where a nullable `normalized` child used to widen the parent dtype silently instead of leaving the incoherence for `validate` to catch. Signed-off-by: Connor Tsui <connor@spiraldb.com> Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
`normalize` was materialising the validity into a `Mask`, then walking it per row to build a second `BufferMut` that differed from `L2Norm`'s output only at the null positions. `fill_null` does that in one call, and it short-circuits to a cast when the input is already non-nullable, so the common path no longer pays for a mask it never reads. The filled norms are the norms child, so the hand-rolled buffer, its `PrimitiveArray::new_unchecked`, and the `execute_mask` call all go away. The row loop drops its validity branch with them: a null row now arrives with a filled zero norm and takes the same path as a genuine zero vector. `ListView::take` fills its offsets and sizes the same way. The `normalized` child keeps its manual zeroing. That buffer is rebuilt element by element for the division regardless, so choosing which value to push costs nothing there. Signed-off-by: Connor Tsui <connor@spiraldb.com> Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Adds `encode_non_nullable` and `encode_nullable` arms to `vortex-tensor/benches/normalized.rs`, so the file covers both directions of the encoding rather than decode alone. They vary width over the same `WIDTHS` as the decode arms, which brackets where the cost lands: a narrow vector leaves the per-row work visible, a wide one buries it under the per-element division. The arms reuse the file's `ELEMENTS` budget, sized for CodSpeed's CPU simulation rather than a desktop, which is what holds every case under the 1 ms per-iteration limit. The benchmark contradicted the previous commit at embedding width, and found two things. Reading the validity off the unexecuted norms array left `L2Norm` to run a second time under the `fill_null` execution, so the cost grew with the tensor width. Canonicalizing first computes the norms once. `fill_null` on a column with no nulls has nothing to fill and still charges a cast, so it is skipped there. Against the hand-rolled mask loop, measured on desktop-sized versions of these arms at dimensions 8 and 768 by the fastest sample over two runs: 12-17% faster at dimension 8, where the numbers are stable to within 0.1% across runs, and neutral at dimension 768, where run-to-run variance of roughly 10% exceeds any difference. Signed-off-by: Connor Tsui <connor@spiraldb.com> Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
force-pushed
the
ct/fix-normalized
branch
from
August 7, 2026 19:38
a71a19a to
91c96d6
Compare
connortsui20
marked this pull request as ready for review
August 7, 2026 19:41
connortsui20
enabled auto-merge (squash)
August 7, 2026 20:29
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
1 participant
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.
Rationale for this change
Normalizedencoding existing issues #9200Normalizedderived its validity by combining two independently nullable children. Decode and read-through paths then had to reconstruct the parent nullability, which caused dtype widening and assertion failures.What changes are included in this PR?
Moves validity onto
Normalizedand makes both data children non-nullable.normalizezeroes both children at null rows, serialization stores an optional validity child, and deserialization rejects layouts whose validity child conflicts with the parent dtype.The lossless split now checks zero rows and zero norms in both directions. The compression scheme rejects non-float tensors, constant-norm decoding only treats exactly
1.0as the identity, andnormalizeavoids recomputing or filling norms when unnecessary.What APIs are changed? Are there any user-facing changes?
This changes the unstable
vortex.tensor.normalizedlayout.try_newandnew_uncheckednow take aValidityand require non-nullable children. The validity child passes through compression unchanged, so nullable columns store the mask directly.