Skip to content

Commit 2a86d43

Browse files
fix(vortex-onpair): remove redundant validate_codes pre-check
OnPair's own decoder already bounds-checks every code in-loop before copying (a near-free, predicted-never-taken branch); vortex-onpair's validate_codes was a second full pass over the codes buffer ahead of decode, doubling memory traffic on a hot path for no additional soundness (out-of-range codes still surface, as a panic instead of a VortexResult error, matching the behavior prior to the onpair 0.1.0 port). Signed-off-by: Francesco Gargiulo <gargiulo.fr@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3da9528 commit 2a86d43

3 files changed

Lines changed: 4 additions & 32 deletions

File tree

encodings/experimental/onpair/src/canonical.rs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ use crate::OnPair;
3131
use crate::OnPairArraySlotsExt;
3232
use crate::decode::code_boundary_at;
3333
use crate::decode::collect_widened;
34-
use crate::decode::validate_codes;
3534

3635
pub(super) fn canonicalize_onpair(
3736
array: ArrayView<'_, OnPair>,
@@ -92,9 +91,6 @@ pub(crate) fn onpair_decode_views(
9291
// boundaries, so an empty boundary slice is sound.
9392
let codes = collect_widened::<u16>(&array.codes().slice(code_start..code_end)?, ctx)?;
9493
let dict_offsets = collect_widened::<u32>(array.dict_offsets(), ctx)?;
95-
// The codes child is file-borne: reject out-of-range codes here so the
96-
// decoder's panicking bounds check never fires on corrupt data.
97-
validate_codes(codes.as_slice(), dict_offsets.len().saturating_sub(1))?;
9894
let dict =
9995
CompactDictionaryView::validate(array.dict_bytes().as_slice(), dict_offsets.as_slice())
10096
.map_err(|e| vortex_err!(InvalidArgument: "Invalid OnPair dictionary: {e}"))?;

encodings/experimental/onpair/src/decode.rs

Lines changed: 4 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -47,23 +47,6 @@ pub(crate) fn code_boundary_at(
4747
.ok_or_else(|| vortex_err!("OnPair codes_offsets[{index}] is null"))
4848
}
4949

50-
/// Ensure every code indexes the dictionary (`code < num_tokens`).
51-
///
52-
/// The slot children are file-borne, so a malformed `codes` child is a
53-
/// recoverable error, not a bug: checking up front keeps the upstream
54-
/// decoder/search primitives — which back their bounds checks with panics —
55-
/// off file-borne data they would panic on.
56-
pub(crate) fn validate_codes(codes: &[u16], num_tokens: usize) -> VortexResult<()> {
57-
// `fold(max)` instead of `Iterator::max`: the latter's last-max-wins
58-
// semantics defeat autovectorization, turning this scan scalar.
59-
let max = codes.iter().fold(0u16, |acc, &c| acc.max(c));
60-
vortex_ensure!(
61-
codes.is_empty() || (max as usize) < num_tokens,
62-
"OnPair code {max} out of range for dictionary of {num_tokens} tokens"
63-
);
64-
Ok(())
65-
}
66-
6750
/// A validated, materialised window over an array's `codes`: the widened
6851
/// per-row `codes_offsets` boundaries plus the codes they bound.
6952
///
@@ -88,9 +71,10 @@ impl CodesWindow {
8871
}
8972
}
9073

91-
/// Materialise and validate the [`CodesWindow`] for every row of `array`:
92-
/// offsets must be nondecreasing and end within the `codes` child, and every
93-
/// code must index the dictionary (see [`validate_codes`]).
74+
/// Materialise the [`CodesWindow`] for every row of `array`: offsets must be
75+
/// nondecreasing and end within the `codes` child. Codes themselves are
76+
/// trusted to the upstream decoder/search primitives, which bounds-check them
77+
/// in-loop and panic on a malformed value.
9478
pub(crate) fn collect_codes_window(
9579
array: ArrayView<'_, OnPair>,
9680
ctx: &mut ExecutionCtx,
@@ -116,10 +100,6 @@ pub(crate) fn collect_codes_window(
116100
array.codes().len()
117101
);
118102
let codes = collect_widened::<u16>(&array.codes().slice(code_start..code_end)?, ctx)?;
119-
validate_codes(
120-
codes.as_slice(),
121-
array.dict_offsets().len().saturating_sub(1),
122-
)?;
123103
Ok(CodesWindow {
124104
offsets,
125105
codes,

encodings/experimental/onpair/src/ops.rs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ use crate::OnPair;
1616
use crate::OnPairArraySlotsExt;
1717
use crate::decode::code_boundary_at;
1818
use crate::decode::collect_widened;
19-
use crate::decode::validate_codes;
2019

2120
impl OperationsVTable<OnPair> for OnPair {
2221
fn scalar_at(
@@ -35,9 +34,6 @@ impl OperationsVTable<OnPair> for OnPair {
3534

3635
let codes = collect_widened::<u16>(&array.codes().slice(row_start..row_end)?, ctx)?;
3736
let dict_offsets = collect_widened::<u32>(array.dict_offsets(), ctx)?;
38-
// The codes child is file-borne: reject out-of-range codes here so the
39-
// decoder's panicking bounds check never fires on corrupt data.
40-
validate_codes(codes.as_slice(), dict_offsets.len().saturating_sub(1))?;
4137
let dict =
4238
CompactDictionaryView::validate(array.dict_bytes().as_slice(), dict_offsets.as_slice())
4339
.map_err(|e| vortex_err!(InvalidArgument: "Invalid OnPair dictionary: {e}"))?;

0 commit comments

Comments
 (0)