Skip to content

Commit d8ce25a

Browse files
ClSlaidClSlaid
committed
revert: drop varbin builder hardening
Signed-off-by: 蔡略 <cailue@apache.org>
1 parent 63698fa commit d8ce25a

4 files changed

Lines changed: 38 additions & 110 deletions

File tree

‎encodings/fsst/src/array.rs‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -315,18 +315,14 @@ impl VTable for FSST {
315315
.validity()?
316316
.execute_mask(array.array().len(), ctx)?;
317317
match_each_integer_ptype!(lengths.ptype(), |P| {
318-
// SAFETY: FSST decoding preserves the source values, and fsst_decode_bytes
319-
// verifies that the decoded byte length matches the per-row lengths.
320-
unsafe {
321-
builder.append_values_unchecked(
322-
bytes.as_slice(),
323-
lengths
324-
.as_slice::<P>()
325-
.iter()
326-
.map(|length| AsPrimitive::<usize>::as_(*length)),
327-
&validity,
328-
);
329-
}
318+
builder.append_values(
319+
bytes.as_slice(),
320+
lengths
321+
.as_slice::<P>()
322+
.iter()
323+
.map(|length| AsPrimitive::<usize>::as_(*length)),
324+
&validity,
325+
);
330326
});
331327
return Ok(());
332328
}

‎encodings/onpair/src/array.rs‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -555,18 +555,14 @@ impl VTable for OnPair {
555555
.validity()?
556556
.execute_mask(array.array().len(), ctx)?;
557557
match_each_integer_ptype!(lengths.ptype(), |P| {
558-
// SAFETY: OnPair decoding preserves the source values and verifies that the
559-
// decoded byte length matches the per-row lengths.
560-
unsafe {
561-
builder.append_values_unchecked(
562-
bytes.as_slice(),
563-
lengths
564-
.as_slice::<P>()
565-
.iter()
566-
.map(|length| AsPrimitive::<usize>::as_(*length)),
567-
&validity,
568-
);
569-
}
558+
builder.append_values(
559+
bytes.as_slice(),
560+
lengths
561+
.as_slice::<P>()
562+
.iter()
563+
.map(|length| AsPrimitive::<usize>::as_(*length)),
564+
&validity,
565+
);
570566
});
571567
return Ok(());
572568
}

‎encodings/zstd/src/array.rs‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1155,26 +1155,20 @@ impl ZstdData {
11551155
match mask.indices() {
11561156
AllOr::All => {
11571157
for value in values {
1158-
// SAFETY: Zstd stores complete source values, preserving UTF-8 validity.
1159-
unsafe {
1160-
builder.append_n_values_unchecked(value, 1);
1161-
}
1158+
builder.append_n_values(value, 1);
11621159
}
11631160
}
11641161
AllOr::None => builder.append_nulls(slice.n_rows),
11651162
AllOr::Some(valid_indices) => {
11661163
let mut row = 0;
11671164
for &valid_index in valid_indices {
11681165
builder.append_nulls(valid_index - row);
1169-
// SAFETY: Zstd stores complete source values, preserving UTF-8 validity.
1170-
unsafe {
1171-
builder.append_n_values_unchecked(
1172-
values
1173-
.next()
1174-
.vortex_expect("Zstd value count must match validity"),
1175-
1,
1176-
);
1177-
}
1166+
builder.append_n_values(
1167+
values
1168+
.next()
1169+
.vortex_expect("Zstd value count must match validity"),
1170+
1,
1171+
);
11781172
row = valid_index + 1;
11791173
}
11801174
builder.append_nulls(slice.n_rows - row);

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

Lines changed: 15 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -113,24 +113,9 @@ impl<O: IntegerPType> VarBinBuilder<O> {
113113
fn append_values_with_lengths(
114114
&mut self,
115115
values: &[u8],
116-
lengths: impl Iterator<Item = usize> + Clone,
116+
lengths: impl Iterator<Item = usize>,
117117
validity: &Mask,
118118
) {
119-
let total_len = lengths
120-
.clone()
121-
.try_fold(0usize, |total, len| total.checked_add(len))
122-
.unwrap_or_else(|| vortex_panic!("VarBin byte length overflow"));
123-
let final_end = self
124-
.data
125-
.len()
126-
.checked_add(total_len)
127-
.unwrap_or_else(|| vortex_panic!("VarBin byte offset overflow"));
128-
O::from(final_end).unwrap_or_else(|| {
129-
vortex_panic!(
130-
"Failed to convert byte offset {final_end} to {}",
131-
std::any::type_name::<O>()
132-
)
133-
});
134119
let mut end = self.data.len();
135120
let mut len = 0;
136121
for value_len in lengths {
@@ -234,16 +219,10 @@ impl VarBinBufferBuilder {
234219
}
235220

236221
/// Appends decompressed values represented by one contiguous byte buffer and per-row lengths.
237-
///
238-
/// # Safety
239-
///
240-
/// `lengths` must contain exactly `validity.len()` items whose sum is `values.len()`.
241-
/// Non-nullable builders require an all-valid mask. For UTF-8 builders, every valid value
242-
/// identified by `lengths` must be valid UTF-8.
243-
pub unsafe fn append_values_unchecked(
222+
pub fn append_values(
244223
&mut self,
245224
values: &[u8],
246-
lengths: impl Iterator<Item = usize> + Clone,
225+
lengths: impl Iterator<Item = usize>,
247226
validity: &Mask,
248227
) {
249228
match &mut self.storage {
@@ -257,11 +236,7 @@ impl VarBinBufferBuilder {
257236
}
258237

259238
/// Appends the same non-null value `n` times.
260-
///
261-
/// # Safety
262-
///
263-
/// For a UTF-8 builder, `value` must be valid UTF-8.
264-
pub unsafe fn append_n_values_unchecked(&mut self, value: impl AsRef<[u8]>, n: usize) {
239+
pub fn append_n_values(&mut self, value: impl AsRef<[u8]>, n: usize) {
265240
let value = value.as_ref();
266241
for _ in 0..n {
267242
self.append_value(value);
@@ -278,13 +253,11 @@ impl VarBinBufferBuilder {
278253
);
279254
match self.dtype() {
280255
DType::Utf8(_) => match scalar.as_utf8().value() {
281-
// SAFETY: Utf8Scalar values are valid UTF-8.
282-
Some(value) => unsafe { self.append_n_values_unchecked(value, n) },
256+
Some(value) => self.append_n_values(value, n),
283257
None => self.push_nulls(n),
284258
},
285259
DType::Binary(_) => match scalar.as_binary().value() {
286-
// SAFETY: Binary builders accept arbitrary bytes.
287-
Some(value) => unsafe { self.append_n_values_unchecked(value, n) },
260+
Some(value) => self.append_n_values(value, n),
288261
None => self.push_nulls(n),
289262
},
290263
dtype => vortex_bail!("VarBinBufferBuilder cannot append scalar of dtype {dtype}"),
@@ -298,31 +271,22 @@ impl VarBinBufferBuilder {
298271
array: crate::ArrayView<'_, VarBin>,
299272
ctx: &mut ExecutionCtx,
300273
) -> VortexResult<()> {
301-
vortex_ensure!(
302-
array.dtype() == self.dtype(),
303-
"VarBinBufferBuilder expected array with dtype {}, got {}",
304-
self.dtype(),
305-
array.dtype()
306-
);
307274
let offsets = array.offsets().clone().execute::<PrimitiveArray>(ctx)?;
308275
let bytes: ByteBuffer = array.sliced_bytes();
309276
let validity = array
310277
.varbin_validity()
311278
.execute_mask(array.as_ref().len(), ctx)?;
312279
crate::match_each_integer_ptype!(offsets.ptype(), |P| {
313280
let offsets = offsets.as_slice::<P>();
314-
// SAFETY: VarBinArray guarantees matching offsets, bytes, validity, and UTF-8.
315-
unsafe {
316-
self.append_values_unchecked(
317-
bytes.as_slice(),
318-
offsets.windows(2).map(|window| {
319-
let start: usize = window[0].as_();
320-
let end: usize = window[1].as_();
321-
end - start
322-
}),
323-
&validity,
324-
);
325-
}
281+
self.append_values(
282+
bytes.as_slice(),
283+
offsets.windows(2).map(|window| {
284+
let start: usize = window[0].as_();
285+
let end: usize = window[1].as_();
286+
end - start
287+
}),
288+
&validity,
289+
);
326290
});
327291
Ok(())
328292
}
@@ -333,12 +297,6 @@ impl VarBinBufferBuilder {
333297
array: crate::ArrayView<'_, VarBinView>,
334298
ctx: &mut ExecutionCtx,
335299
) -> VortexResult<()> {
336-
vortex_ensure!(
337-
array.dtype() == self.dtype(),
338-
"VarBinBufferBuilder expected array with dtype {}, got {}",
339-
self.dtype(),
340-
array.dtype()
341-
);
342300
let validity = array
343301
.varbinview_validity()
344302
.execute_mask(array.as_ref().len(), ctx)?;
@@ -517,22 +475,6 @@ mod tests {
517475
Ok(())
518476
}
519477

520-
#[test]
521-
fn test_safe_append_rejects_dtype_mismatch() {
522-
let mut builder = VarBinBufferBuilder::with_capacity(DType::Utf8(Nullable), false, 1);
523-
let mut ctx = array_session().create_execution_ctx();
524-
let varbin = VarBinArray::from_iter([Some(b"hello".as_slice())], DType::Binary(Nullable));
525-
assert!(builder.append_varbin(varbin.as_view(), &mut ctx).is_err());
526-
527-
let varbinview =
528-
VarBinViewArray::from_iter([Some(b"hello".as_slice())], DType::Binary(Nullable));
529-
assert!(
530-
builder
531-
.append_varbinview(varbinview.as_view(), &mut ctx)
532-
.is_err()
533-
);
534-
}
535-
536478
#[rstest]
537479
#[case(false)]
538480
#[case(true)]

0 commit comments

Comments
 (0)