Skip to content

Commit 7216cb5

Browse files
authored
Validate VarBin arrays with an explicit execution context (#10593)
- The VarBin constructors take an `ExecutionCtx` to validate offsets and UTF-8 data. This covers `VarBinArray::{new, try_new}` and `VarBinData::{build, build_from_handle, try_build, try_build_from_handle, validate}`. - UTF-8 validation stays complete, because other code reads these bytes with `from_utf8_unchecked`. - Deserialization validates with a context from its session. - `offset_at`, `bytes_at` and `sliced_bytes` take a ctx. - The VarBin and FSST nullability casts don't change offsets or bytes, so they rebuild with `new_unchecked_from_handle` instead of revalidating. - The builder's debug sortedness check reads its own offsets slice directly. part of #9446
1 parent e7fd0f6 commit 7216cb5

20 files changed

Lines changed: 128 additions & 78 deletions

File tree

‎encodings/fsst/benches/fsst_url_compare.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ fn pick_url_with_domain(data: &VarBinArray, domain: &str) -> String {
7070
.unwrap();
7171
(0..data.len())
7272
.filter(|&i| mask.value(i))
73-
.map(|i| unsafe { String::from_utf8_unchecked(data.bytes_at(i).to_vec()) })
73+
.map(|i| unsafe { String::from_utf8_unchecked(data.bytes_at(i, &mut ctx).to_vec()) })
7474
.find(|u| u.contains(domain))
7575
.unwrap_or_else(|| format!("http://{domain}/missing"))
7676
}

‎encodings/fsst/src/canonical.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ mod tests {
301301
let arr = builder.finish_into_varbin();
302302
let mask = arr.validity()?.execute_mask(arr.len(), &mut ctx)?;
303303
let actual = (0..arr.len())
304-
.map(|i| mask.value(i).then(|| arr.bytes_at(i).to_vec()))
304+
.map(|i| mask.value(i).then(|| arr.bytes_at(i, &mut ctx).to_vec()))
305305
.collect::<Vec<_>>();
306306
assert_eq!(data, actual);
307307
}
@@ -368,6 +368,7 @@ mod tests {
368368
encoded.codes_bytes().clone(),
369369
DType::Binary(Nullability::NonNullable),
370370
Validity::NonNullable,
371+
&mut ctx,
371372
);
372373
let sliced = FSST::try_new_with_symbol_table(
373374
encoded.dtype().clone(),

‎encodings/fsst/src/compute/cast.rs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,16 @@ fn build_with_codes_validity(
2323
new_codes_validity: Validity,
2424
) -> VortexResult<ArrayRef> {
2525
let codes = array.codes();
26-
let new_codes = VarBinArray::try_new(
27-
codes.offsets().clone(),
28-
codes.bytes().clone(),
29-
codes.dtype().with_nullability(dtype.nullability()),
30-
new_codes_validity,
31-
)?;
26+
// SAFETY: the offsets and bytes are unchanged from valid codes, and the validity was cast
27+
// from the codes' own validity, so it has the same length.
28+
let new_codes = unsafe {
29+
VarBinArray::new_unchecked_from_handle(
30+
codes.offsets().clone(),
31+
codes.bytes_handle().clone(),
32+
codes.dtype().with_nullability(dtype.nullability()),
33+
new_codes_validity,
34+
)
35+
};
3236

3337
Ok(unsafe {
3438
FSST::new_unchecked_with_symbol_table(

‎encodings/zstd/src/array.rs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1610,6 +1610,8 @@ impl OperationsVTable<Zstd> for Zstd {
16101610
#[expect(clippy::cast_possible_truncation)]
16111611
mod tests {
16121612
use rstest::rstest;
1613+
use vortex_array::VortexSessionExecute;
1614+
use vortex_array::array_session;
16131615
use vortex_array::arrays::varbin::VarBinArrayExt as _;
16141616
use vortex_array::builders::VarBinBuilder;
16151617
use vortex_array::dtype::DType;
@@ -1779,8 +1781,9 @@ mod tests {
17791781
append_slice_to_varbin(&slice, &Mask::new_true(2), &mut builder)?;
17801782

17811783
let appended = builder.finish_into_varbin();
1782-
assert_eq!(appended.bytes_at(0).as_slice(), b"hello");
1783-
assert_eq!(appended.bytes_at(1).as_slice(), b"world");
1784+
let mut ctx = array_session().create_execution_ctx();
1785+
assert_eq!(appended.bytes_at(0, &mut ctx).as_slice(), b"hello");
1786+
assert_eq!(appended.bytes_at(1, &mut ctx).as_slice(), b"world");
17841787
Ok(())
17851788
}
17861789

‎vortex-array/benches/dict_compare.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ fn bench_compare_varbin(bencher: divan::Bencher, (len, uniqueness): (usize, usiz
7777
let mut ctx = SESSION.create_execution_ctx();
7878
let varbin_arr = VarBinArray::from(gen_varbin_words(len, uniqueness));
7979
let dict = dict_encode(&varbin_arr.clone().into_array(), &mut ctx).unwrap();
80-
let const_bytes = varbin_arr.bytes_at(0);
80+
let const_bytes = varbin_arr.bytes_at(0, &mut ctx);
8181
let value = unsafe { str::from_utf8_unchecked(const_bytes.as_slice()) };
8282

8383
bencher
@@ -155,7 +155,7 @@ fn bench_compare_sliced_dict_varbinview(
155155
let varbin_arr = VarBinArray::from(gen_varbin_words(codes_len.max(values_len), values_len));
156156
let dict = dict_encode(&varbin_arr.clone().into_array(), &mut ctx).unwrap();
157157
let dict = dict.into_array().slice(0..codes_len).unwrap();
158-
let const_bytes = varbin_arr.bytes_at(0);
158+
let const_bytes = varbin_arr.bytes_at(0, &mut ctx);
159159
let value = unsafe { str::from_utf8_unchecked(const_bytes.as_slice()) };
160160

161161
bencher

‎vortex-array/src/arrays/validation_tests.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ mod tests {
151151
#[case] validity: Validity,
152152
#[case] ok: bool,
153153
) {
154+
let mut ctx = array_session().create_execution_ctx();
154155
let nullability = if matches!(validity, Validity::NonNullable) {
155156
Nullability::NonNullable
156157
} else {
@@ -161,6 +162,7 @@ mod tests {
161162
ByteBuffer::from(bytes),
162163
DType::Utf8(nullability),
163164
validity,
165+
&mut ctx,
164166
);
165167
assert_eq!(result.is_ok(), ok);
166168
}

‎vortex-array/src/arrays/varbin/array.rs‎

Lines changed: 47 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ use vortex_error::vortex_err;
1818
use crate::ArrayRef;
1919
use crate::ArraySlots;
2020
use crate::ExecutionCtx;
21-
use crate::VortexSessionExecute;
2221
use crate::aggregate_fn::NumericalAggregateOpts;
2322
use crate::aggregate_fn::fns::min_max::min_max;
2423
use crate::array::Array;
@@ -33,7 +32,6 @@ use crate::buffer::BufferHandle;
3332
use crate::dtype::DType;
3433
use crate::dtype::Nullability;
3534
use crate::dtype::OffsetBuilderPType;
36-
use crate::legacy_session;
3735
use crate::match_each_integer_ptype;
3836
use crate::validity::Validity;
3937

@@ -102,8 +100,14 @@ impl VarBinData {
102100
///
103101
/// Panics if the provided components do not satisfy the invariants documented
104102
/// in `VarBinArray::new_unchecked`.
105-
pub fn build(offsets: ArrayRef, bytes: ByteBuffer, dtype: DType, validity: Validity) -> Self {
106-
Self::try_build(offsets, bytes, dtype, validity).vortex_expect("VarBinArray new")
103+
pub fn build(
104+
offsets: ArrayRef,
105+
bytes: ByteBuffer,
106+
dtype: DType,
107+
validity: Validity,
108+
ctx: &mut ExecutionCtx,
109+
) -> Self {
110+
Self::try_build(offsets, bytes, dtype, validity, ctx).vortex_expect("VarBinArray new")
107111
}
108112

109113
/// Creates a new `VarBinArray`.
@@ -117,8 +121,10 @@ impl VarBinData {
117121
bytes: BufferHandle,
118122
dtype: DType,
119123
validity: Validity,
124+
ctx: &mut ExecutionCtx,
120125
) -> Self {
121-
Self::try_build_from_handle(offset, bytes, dtype, validity).vortex_expect("VarBinArray new")
126+
Self::try_build_from_handle(offset, bytes, dtype, validity, ctx)
127+
.vortex_expect("VarBinArray new")
122128
}
123129

124130
pub(crate) fn make_slots(offsets: ArrayRef, validity: &Validity, len: usize) -> ArraySlots {
@@ -142,9 +148,10 @@ impl VarBinData {
142148
bytes: ByteBuffer,
143149
dtype: DType,
144150
validity: Validity,
151+
ctx: &mut ExecutionCtx,
145152
) -> VortexResult<Self> {
146153
let bytes = BufferHandle::new_host(bytes);
147-
Self::validate(&offsets, &bytes, &dtype, &validity)?;
154+
Self::validate(&offsets, &bytes, &dtype, &validity, ctx)?;
148155

149156
// SAFETY: validate ensures all invariants are met.
150157
Ok(unsafe { Self::new_unchecked_from_handle(bytes) })
@@ -164,8 +171,9 @@ impl VarBinData {
164171
bytes: BufferHandle,
165172
dtype: DType,
166173
validity: Validity,
174+
ctx: &mut ExecutionCtx,
167175
) -> VortexResult<Self> {
168-
Self::validate(&offsets, &bytes, &dtype, &validity)?;
176+
Self::validate(&offsets, &bytes, &dtype, &validity, ctx)?;
169177

170178
// SAFETY: validate ensures all invariants are met.
171179
Ok(unsafe { Self::new_unchecked_from_handle(bytes) })
@@ -218,12 +226,12 @@ impl VarBinData {
218226
/// Validates the components that would be used to create a `VarBinArray`.
219227
///
220228
/// This function checks all the invariants required by `VarBinArray::new_unchecked`.
221-
#[allow(clippy::disallowed_methods)]
222229
pub fn validate(
223230
offsets: &ArrayRef,
224231
bytes: &BufferHandle,
225232
dtype: &DType,
226233
validity: &Validity,
234+
ctx: &mut ExecutionCtx,
227235
) -> VortexResult<()> {
228236
// Check offsets are non-nullable integer
229237
vortex_ensure!(
@@ -267,10 +275,9 @@ impl VarBinData {
267275
&& matches!(dtype, DType::Utf8(_))
268276
&& let Some(bytes) = bytes.as_host_opt()
269277
{
270-
Self::validate_utf8(offsets, bytes.as_ref(), validity)?;
278+
Self::validate_utf8(offsets, bytes.as_ref(), validity, ctx)?;
271279
} else if offsets.is_host() {
272-
let mut ctx = legacy_session().create_execution_ctx();
273-
Self::validate_offsets(offsets, bytes.len(), &mut ctx)?;
280+
Self::validate_offsets(offsets, bytes.len(), ctx)?;
274281
}
275282

276283
Ok(())
@@ -328,8 +335,12 @@ impl VarBinData {
328335
}
329336

330337
/// Validates that every non-null value is valid UTF-8.
331-
#[allow(clippy::disallowed_methods)]
332-
fn validate_utf8(offsets: &ArrayRef, bytes: &[u8], validity: &Validity) -> VortexResult<()> {
338+
fn validate_utf8(
339+
offsets: &ArrayRef,
340+
bytes: &[u8],
341+
validity: &Validity,
342+
ctx: &mut ExecutionCtx,
343+
) -> VortexResult<()> {
333344
let validate_at = |i: usize, start: usize, end: usize| -> VortexResult<()> {
334345
let string_bytes = bytes.get(start..end).ok_or_else(|| {
335346
vortex_err!(
@@ -346,16 +357,15 @@ impl VarBinData {
346357
Ok(())
347358
};
348359

349-
let mut ctx = legacy_session().create_execution_ctx();
350360
// TODO(joe): update the created VarBin with this decompressed Array.
351-
let primitive_offsets = offsets.clone().execute::<PrimitiveArray>(&mut ctx)?;
361+
let primitive_offsets = offsets.clone().execute::<PrimitiveArray>(ctx)?;
352362

353363
// Array-backed validity is the only variant that needs an execution context: execute it into
354364
// a mask once. The constant variants resolve null-ness without one. Resolving this before
355365
// the per-type dispatch keeps the dtype loop simple.
356366
let mask = match validity {
357367
Validity::Array(_) => {
358-
Some(validity.execute_mask(primitive_offsets.len().saturating_sub(1), &mut ctx)?)
368+
Some(validity.execute_mask(primitive_offsets.len().saturating_sub(1), ctx)?)
359369
}
360370
_ => None,
361371
};
@@ -378,7 +388,7 @@ impl VarBinData {
378388

379389
// The fast path proves the offsets are valid. The per-string check below skips null
380390
// rows, so it cannot find bad offsets at those rows.
381-
Self::validate_offsets(offsets, bytes.len(), &mut ctx)?;
391+
Self::validate_offsets(offsets, bytes.len(), ctx)?;
382392

383393
// Invalid bytes at a null row fail the check above, so check valid strings one by one.
384394
for (i, (start, end)) in offsets_slice
@@ -438,8 +448,7 @@ pub trait VarBinArrayExt: VarBinArraySlotsExt {
438448
)
439449
}
440450

441-
#[allow(clippy::disallowed_methods)]
442-
fn offset_at(&self, index: usize) -> usize {
451+
fn offset_at(&self, index: usize, ctx: &mut ExecutionCtx) -> usize {
443452
assert!(
444453
index <= self.len(),
445454
"Index {index} out of bounds 0..={}",
@@ -448,21 +457,21 @@ pub trait VarBinArrayExt: VarBinArraySlotsExt {
448457

449458
(&self
450459
.offsets()
451-
.execute_scalar(index, &mut legacy_session().create_execution_ctx())
460+
.execute_scalar(index, ctx)
452461
.vortex_expect("offsets must support execute_scalar"))
453462
.try_into()
454463
.vortex_expect("Failed to convert offset to usize")
455464
}
456465

457-
fn bytes_at(&self, index: usize) -> ByteBuffer {
458-
let start = self.offset_at(index);
459-
let end = self.offset_at(index + 1);
466+
fn bytes_at(&self, index: usize, ctx: &mut ExecutionCtx) -> ByteBuffer {
467+
let start = self.offset_at(index, ctx);
468+
let end = self.offset_at(index + 1, ctx);
460469
self.bytes().slice(start..end)
461470
}
462471

463-
fn sliced_bytes(&self) -> ByteBuffer {
464-
let first_offset: usize = self.offset_at(0);
465-
let last_offset = self.offset_at(self.len());
472+
fn sliced_bytes(&self, ctx: &mut ExecutionCtx) -> ByteBuffer {
473+
let first_offset: usize = self.offset_at(0, ctx);
474+
let last_offset = self.offset_at(self.len(), ctx);
466475
self.bytes().slice(first_offset..last_offset)
467476
}
468477
}
@@ -567,7 +576,13 @@ impl Array<VarBin> {
567576

568577
impl Array<VarBin> {
569578
/// Creates a new `VarBinArray`.
570-
pub fn new(offsets: ArrayRef, bytes: ByteBuffer, dtype: DType, validity: Validity) -> Self {
579+
pub fn new(
580+
offsets: ArrayRef,
581+
bytes: ByteBuffer,
582+
dtype: DType,
583+
validity: Validity,
584+
ctx: &mut ExecutionCtx,
585+
) -> Self {
571586
let len = offsets.len().saturating_sub(1);
572587
let slots = VarBinData::make_slots(offsets, &validity, len);
573588
let data = VarBinData::build(
@@ -578,6 +593,7 @@ impl Array<VarBin> {
578593
bytes,
579594
dtype.clone(),
580595
validity,
596+
ctx,
581597
);
582598
unsafe { Array::from_parts_unchecked(ArrayParts::new(VarBin, dtype, len, data, slots)) }
583599
}
@@ -616,16 +632,18 @@ impl Array<VarBin> {
616632
unsafe { Array::from_parts_unchecked(ArrayParts::new(VarBin, dtype, len, data, slots)) }
617633
}
618634

619-
/// Constructs a new `VarBinArray`.
635+
/// Constructs a new `VarBinArray`, using `ctx` to validate the offsets and, for UTF-8 arrays,
636+
/// the string data.
620637
pub fn try_new(
621638
offsets: ArrayRef,
622639
bytes: ByteBuffer,
623640
dtype: DType,
624641
validity: Validity,
642+
ctx: &mut ExecutionCtx,
625643
) -> VortexResult<Self> {
626644
let len = offsets.len() - 1;
627645
let bytes = BufferHandle::new_host(bytes);
628-
VarBinData::validate(&offsets, &bytes, &dtype, &validity)?;
646+
VarBinData::validate(&offsets, &bytes, &dtype, &validity, ctx)?;
629647
let slots = VarBinData::make_slots(offsets, &validity, len);
630648
// SAFETY: validate ensures all invariants are met.
631649
let data = unsafe { VarBinData::new_unchecked_from_handle(bytes) };

‎vortex-array/src/arrays/varbin/builder.rs‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@ use crate::ArrayView;
2525
use crate::Canonical;
2626
use crate::ExecutionCtx;
2727
use crate::IntoArray;
28-
#[cfg(debug_assertions)]
29-
use crate::VortexSessionExecute;
3028
use crate::arrays::PrimitiveArray;
3129
use crate::arrays::VarBin;
3230
use crate::arrays::VarBinArray;
@@ -39,8 +37,6 @@ use crate::dtype::DType;
3937
use crate::dtype::OffsetBuilderPType;
4038
use crate::expr::stats::Precision;
4139
use crate::expr::stats::Stat;
42-
#[cfg(debug_assertions)]
43-
use crate::legacy_session;
4440
use crate::match_each_integer_ptype;
4541
use crate::scalar::Scalar;
4642
use crate::validity::Validity;
@@ -352,7 +348,7 @@ impl<O: OffsetBuilderPType> VarBinBuilder<O> {
352348
usize: AsPrimitive<O>,
353349
{
354350
let offsets = array.offsets().clone().execute::<PrimitiveArray>(ctx)?;
355-
let bytes: ByteBuffer = array.sliced_bytes();
351+
let bytes: ByteBuffer = array.sliced_bytes(ctx);
356352
let validity = array
357353
.varbin_validity()
358354
.execute_mask(array.as_ref().len(), ctx)?;
@@ -406,7 +402,6 @@ impl<O: OffsetBuilderPType> VarBinBuilder<O> {
406402
}
407403

408404
/// Finishes the appended values into a [`VarBinArray`] and resets the builder.
409-
#[allow(clippy::disallowed_methods)]
410405
pub fn finish_into_varbin(&mut self) -> VarBinArray {
411406
assert_eq!(
412407
self.offsets.len() - 1,
@@ -424,14 +419,10 @@ impl<O: OffsetBuilderPType> VarBinBuilder<O> {
424419

425420
// The builder adds offsets in monotonically increasing order. Store this statistic to
426421
// prevent VarBinArray::validate from recomputing it after deserialization.
427-
#[cfg(debug_assertions)]
428-
{
429-
let offsets_are_sorted = offsets
430-
.statistics()
431-
.compute_is_sorted(&mut legacy_session().create_execution_ctx())
432-
.unwrap_or(false);
433-
debug_assert!(offsets_are_sorted, "VarBinBuilder offsets must be sorted");
434-
}
422+
debug_assert!(
423+
offsets.as_slice::<O>().is_sorted(),
424+
"VarBinBuilder offsets must be sorted"
425+
);
435426
offsets
436427
.statistics()
437428
.set(Stat::IsSorted, Precision::Exact(true.into()));

0 commit comments

Comments
 (0)