Repository navigation
Avoid truncating dictionary fill codes - #10745
Conversation
Merging this PR will degrade performance by 5.18%4 warnings, 1 note
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
| if fill_index > codes_dtype.as_ptype().max_value_as_u64() { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
should we widen instead of just giving up here?
There was a problem hiding this comment.
we could widen here, since it just copies the codes. Can make this a follow up
The dictionary fill_null kernel converted the index of the fill value into the codes type with an unchecked cast. A dictionary can hold more values than its codes type can index, so an unreferenced fill value could truncate to the wrong code, or panic for signed codes. The kernel now checks the index against the range of the codes and declines when it does not fit, so the generic path fills the decoded array. Non-nullable codes need no fill code, so they keep the fast path. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
5602cd0 to
5c0ae8e
Compare
## Summary - Tracking Issue: #10804 Follow-up to #10745, as suggested in review there. When the fill value already exists in the dictionary values but its index does not fit the codes type (e.g. `u8` codes and the fill value at index 300), the `Dict` `fill_null` kernel returned `None`, so the generic path decoded the whole array before filling. The kernel only rewrites the null codes, so it can widen them instead and keep the result dictionary-encoded. ## Changes - For nullable codes whose type cannot hold the fill index, cast the codes to `PType::min_unsigned_ptype_for_value(fill_index)` (kept nullable) and then fill the nulls with the index. Codes are never negative, so the cast keeps every code. Codes that already fit and non-nullable codes take the same path as before. - New test `fill_unreferenced_value_widens_codes` calls the kernel directly and checks that it returns a `Dict` with widened, non-nullable codes and the right values, for `u8 -> u16` (256, 300), `i8 -> u8` (128), `i8 -> u16` (256) and `u16 -> u32` (65536). All five cases fail on `develop`, where the kernel returns `None`. Tested with `cargo test -p vortex-array --lib` (4437 passed), `cargo clippy -p vortex-array --all-targets --all-features -- -D warnings`, and `cargo +nightly-2026-09-10 fmt -p vortex-array`. AI assistance: I used an AI coding assistant to help write this change and the description; I reviewed the code and ran the tests above myself. Signed-off-by: Vincent <1729065730@qq.com>
Summary
Fixes #10623.
Dict::fill_nullconverted the index of the matching dictionary value into the codes type with an uncheckedascast. A dictionary can hold more values than its codes type can index, so an unreferenced fill value could truncate to the wrong code (withu8codes, filling with value 300 returned value 44) or panic for signed codes.Changes
The kernel now checks the index against the range of the codes type. When the index does not fit, the kernel declines and the generic
fill_nullpath fills the decoded array, which is slower but correct. Non-nullable codes need no fill code, so they keep the fast path. The new tests coveru8andi8codes at and past their maximum, and non-nullable codes over nullable values. The three cases past the maximum fail without the fix.The
Narrowwork (#10733) depends on this fix, because a rule there makes narrow dictionary codes common.🤖 Generated with Claude Code