Skip to content

Commit 7507307

Browse files
committed
Reuse validated chunk boundaries during lowering
Copy the already validated chunk start offsets into the existing Concat Arc instead of repeatedly asking the layout for each child's row count. Keep V1's offset storage and the base planner/scheduler unchanged. This is a measured deviation from PR #10540, isolated for comparison with 5231b70. Four regression cases cover regular, empty, absent, and zero-row chunks; three fail before the change. No child layouts are materialized to lower the boundaries. Production change is +7/-10 lines in two files. Local four-round, five-second standard comparisons show clustered feature vectors improving 8.0% cached / 3.5% reopen on one core and 17.1% / 4.9% on 30 cores. The initial taxi cached +6.4% reduced to +0.8% in the longer run. Nested-struct reopen is still +4.9%; identical-binary controls vary up to 7.8%, so that result is unresolved rather than dismissed as noise. Sixteen full-Arrow list cases initially show at most +2.01% slowdown; longer list confirmation and isolated parent CI remain pending. Repeated memory checks show no increase in taxi maximum live allocation, and median requested read counts/bytes are unchanged. No all-cases V1 win is claimed. Validation: 618 layout tests, all-target/all-feature layout Clippy with -D warnings, pinned formatting, whitespace, release benchmark builds, and 72 real-file V1 full-scan-plus-take correctness comparisons passed. No workspace-wide checks or doctests were run. Keep separate from the draft PR until the exact-parent CI comparison is assessed. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
1 parent 5231b70 commit 7507307

3 files changed

Lines changed: 65 additions & 10 deletions

File tree

‎vortex-layout/src/layouts/chunked/mod.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,11 @@ impl Layout<Chunked> {
125125
OwnedLayoutChildren::layout_children(children),
126126
)
127127
}
128+
129+
/// Validated chunk boundaries, including the terminal row count.
130+
pub(crate) fn chunk_offsets(&self) -> &[u64] {
131+
&self.chunk_offsets
132+
}
128133
}
129134

130135
fn chunk_offsets(children: &dyn LayoutChildren) -> VortexResult<Vec<u64>> {

‎vortex-layout/src/plan/physical/lower.rs‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,21 +83,13 @@ fn lower_flat(layout: &FlatLayout) -> SegmentScanPlan {
8383
}
8484

8585
fn lower_chunked(layout: &ChunkedLayout) -> VortexResult<ConcatPlan> {
86-
let mut row_offsets = Vec::with_capacity(layout.nchildren());
87-
let mut row_count = 0u64;
88-
for index in 0..layout.nchildren() {
89-
row_offsets.push(row_count);
90-
row_count = row_count
91-
.checked_add(layout.child_row_count(index))
92-
.ok_or_else(|| vortex_err!("Chunked row count overflow"))?;
93-
}
9486
// SAFETY: Chunked layout construction validates that every child has the parent dtype and
95-
// that their row counts sum to the parent; offsets were computed with checked addition above.
87+
// that these checked cumulative offsets end at the parent's row count.
9688
Ok(unsafe {
9789
ConcatPlan::from_children_unchecked(
9890
layout.dtype().clone(),
9991
layout.row_count(),
100-
row_offsets.into(),
92+
layout.chunk_offsets()[..layout.nchildren()].into(),
10193
lazy_children(layout.to_layout(), (0..layout.nchildren()).collect()),
10294
)
10395
})

‎vortex-layout/src/plan/physical/tests.rs‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@
44
use std::fmt;
55
use std::num::NonZeroUsize;
66
use std::sync::Arc;
7+
use std::sync::atomic::AtomicUsize;
8+
use std::sync::atomic::Ordering;
79

10+
use rstest::rstest;
811
use vortex_array::IntoArray;
912
use vortex_array::VortexSessionExecute;
1013
use vortex_array::aggregate_fn::AggregateFnRef;
@@ -43,6 +46,7 @@ use vortex_session::registry::ReadContext;
4346

4447
use super::*;
4548
use crate::LayoutBuildContext;
49+
use crate::LayoutChildren;
4650
use crate::LayoutEncoding;
4751
use crate::LayoutRef;
4852
use crate::OwnedLayoutChildren;
@@ -150,6 +154,60 @@ fn chunked_plan_exposes_chunks() -> VortexResult<()> {
150154
Ok(())
151155
}
152156

157+
#[derive(Clone)]
158+
struct CountedChunkRows {
159+
rows: Arc<[u64]>,
160+
calls: Arc<AtomicUsize>,
161+
}
162+
163+
impl LayoutChildren for CountedChunkRows {
164+
fn to_arc(&self) -> Arc<dyn LayoutChildren> {
165+
Arc::new(self.clone())
166+
}
167+
168+
fn child(&self, _idx: usize, _dtype: &DType) -> VortexResult<LayoutRef> {
169+
Err(vortex_err!("Lowering must leave chunk children lazy"))
170+
}
171+
172+
fn child_row_count(&self, idx: usize) -> u64 {
173+
self.calls.fetch_add(1, Ordering::Relaxed);
174+
self.rows[idx]
175+
}
176+
177+
fn nchildren(&self) -> usize {
178+
self.rows.len()
179+
}
180+
}
181+
182+
#[rstest]
183+
#[case::normal(vec![3, 2], vec![0, 3])]
184+
#[case::empty_chunks(vec![0, 2, 0, 3, 0], vec![0, 0, 2, 2, 5])]
185+
#[case::empty(vec![], vec![])]
186+
#[case::zero_rows(vec![0], vec![0])]
187+
fn lowering_reuses_validated_chunk_boundaries(
188+
#[case] rows: Vec<u64>,
189+
#[case] offsets: Vec<u64>,
190+
) -> VortexResult<()> {
191+
let calls = Arc::new(AtomicUsize::new(0));
192+
let row_count = rows.iter().sum();
193+
let layout = ChunkedLayout::new(
194+
row_count,
195+
DType::Null,
196+
Arc::new(CountedChunkRows {
197+
rows: rows.into(),
198+
calls: Arc::clone(&calls),
199+
}),
200+
)
201+
.into_layout();
202+
calls.store(0, Ordering::Relaxed);
203+
let plan = lower(&layout)?;
204+
assert_eq!(plan.as_::<Concat>().row_offsets(), offsets);
205+
assert_eq!(plan.row_count(), row_count);
206+
assert_eq!(plan.child_count(), offsets.len());
207+
assert_eq!(calls.load(Ordering::Relaxed), 0);
208+
Ok(())
209+
}
210+
153211
#[test]
154212
fn chunked_plan_lowers_each_chunk_on_access() -> VortexResult<()> {
155213
let dtype = primitive(PType::I32, Nullability::NonNullable);

0 commit comments

Comments
 (0)