Repository navigation
Avoid truncating dictionary fill codes - #10745
connortsui20 wants to merge 1 commit into
Conversation
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>
Merging this PR will degrade performance by 4.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?
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