Skip to content

Commit 39a4e35

Browse files
robert3005claude
andcommitted
refactor(array): drop append_to_builder_unchecked
Chunked appends its chunks through the checked `append_to_builder` again, paying the builder dtype comparison and the length post-condition per chunk rather than once for the whole chunked array. Signed-off-by: Robert Kruszewski <robert@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CZTu9XUrdRZZ91rx852DGf
1 parent e1a7d80 commit 39a4e35

3 files changed

Lines changed: 3 additions & 48 deletions

File tree

‎vortex-array/src/array/erased.rs‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -421,27 +421,6 @@ impl ArrayRef {
421421
self.0.data.append_to_builder(self, builder, ctx)
422422
}
423423

424-
/// [`append_to_builder`](Self::append_to_builder) for a caller that is itself inside an
425-
/// [`append_to_builder`](Self::append_to_builder), and so is already covered by its dtype check
426-
/// and its length post-condition.
427-
///
428-
/// Appending the chunks of a [`ChunkedArray`](crate::arrays::ChunkedArray) is the case this
429-
/// exists for. The chunked array's own dtype was checked on the way in and every chunk shares
430-
/// it, and if the chunks together grow the builder by the wrong number of values, the enclosing
431-
/// check on the chunked array says so — both are worth paying once rather than once per chunk.
432-
pub(crate) fn append_to_builder_unchecked(
433-
&self,
434-
builder: &mut dyn ArrayBuilder,
435-
ctx: &mut ExecutionCtx,
436-
) -> VortexResult<()> {
437-
debug_assert_eq!(
438-
builder.dtype(),
439-
self.dtype(),
440-
"append_to_builder_unchecked called with a mismatched builder",
441-
);
442-
self.0.data.append_to_builder_unchecked(self, builder, ctx)
443-
}
444-
445424
/// Returns the statistics of the array.
446425
pub fn statistics(&self) -> StatsSetRef<'_> {
447426
self.0.stats.to_ref(self)

‎vortex-array/src/array/mod.rs‎

Lines changed: 2 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -137,18 +137,6 @@ pub(crate) trait DynArrayData: 'static + private::Sealed + Send + Sync + Debug {
137137
ctx: &mut ExecutionCtx,
138138
) -> VortexResult<()>;
139139

140-
/// [`append_to_builder`](Self::append_to_builder) without its dtype check or its length
141-
/// post-condition.
142-
///
143-
/// The caller must have established that `builder.dtype() == this.dtype()`, and is responsible
144-
/// for checking that the builder grew by the number of values it was handed.
145-
fn append_to_builder_unchecked(
146-
&self,
147-
this: &ArrayRef,
148-
builder: &mut dyn ArrayBuilder,
149-
ctx: &mut ExecutionCtx,
150-
) -> VortexResult<()>;
151-
152140
// --- Visitor methods (formerly in ArrayVisitor) ---
153141

154142
/// Returns the buffers of the array.
@@ -320,7 +308,8 @@ impl<V: VTable> DynArrayData for ArrayData<V> {
320308
}
321309
let len = builder.len();
322310

323-
self.append_to_builder_unchecked(this, builder, ctx)?;
311+
let view = unsafe { ArrayView::new_unchecked(this, &self.data) };
312+
V::append_to_builder(view, builder, ctx)?;
324313

325314
assert_eq!(
326315
len + this.len(),
@@ -331,16 +320,6 @@ impl<V: VTable> DynArrayData for ArrayData<V> {
331320
Ok(())
332321
}
333322

334-
fn append_to_builder_unchecked(
335-
&self,
336-
this: &ArrayRef,
337-
builder: &mut dyn ArrayBuilder,
338-
ctx: &mut ExecutionCtx,
339-
) -> VortexResult<()> {
340-
let view = unsafe { ArrayView::new_unchecked(this, &self.data) };
341-
V::append_to_builder(view, builder, ctx)
342-
}
343-
344323
fn buffers(&self, this: &ArrayRef) -> Vec<ByteBuffer> {
345324
let view = unsafe { ArrayView::new_unchecked(this, &self.data) };
346325
(0..V::nbuffers(view))

‎vortex-array/src/arrays/chunked/vtable/mod.rs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -242,11 +242,8 @@ impl VTable for Chunked {
242242
// The builder is about to receive one array per chunk, so let any chunk lists it keeps
243243
// grow once rather than on the way through.
244244
builder.reserve_chunks(array.nchunks());
245-
// The dtype check and the length post-condition around this call already cover the chunks:
246-
// every chunk shares this array's dtype, and their lengths sum to its own. Paying for both
247-
// once beats paying per chunk.
248245
for chunk in array.iter_chunks() {
249-
chunk.append_to_builder_unchecked(builder, ctx)?;
246+
chunk.append_to_builder(builder, ctx)?;
250247
}
251248
Ok(())
252249
}

0 commit comments

Comments
 (0)