Skip to content

Commit d092cfd

Browse files
committed
refactor(layout): keep Filter out of lower, and let a segment scan keep its own rows
A flat layout lowers to a bare segment scan again, as on develop. Every plan now produces only the rows its split selects, a segment scan included, so the scan's source applies the mask; a Filter over a scan compiles to the same source. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NWYmfHTFtaEdhdyd3u5hv3
1 parent 7856717 commit d092cfd

4 files changed

Lines changed: 89 additions & 157 deletions

File tree

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ use crate::layouts::list::VALIDITY_CHILD_INDEX;
2525
use crate::layouts::struct_::Struct;
2626
use crate::layouts::struct_::StructLayout;
2727
use crate::plan::ConcatPlan;
28-
use crate::plan::FilterPlan;
2928
use crate::plan::ListPackPlan;
3029
use crate::plan::PackPlan;
3130
use crate::plan::PlanChildren;
@@ -37,12 +36,9 @@ use crate::plan::TakePlan;
3736
///
3837
/// The root operator is built immediately. Its child container owns a hidden clone of the source
3938
/// layout and lowers each child independently on first access.
40-
///
41-
/// A flat layout lowers to a [`Filter`](crate::plan::Filter) over its segment scan, so the scan
42-
/// returns only the rows it is executed with.
4339
pub fn lower(layout: &LayoutRef) -> VortexResult<PlanRef> {
4440
if let Some(layout) = layout.as_opt::<Flat>() {
45-
return Ok(FilterPlan::new(lower_flat(layout).into_plan()).into_plan());
41+
return Ok(lower_flat(layout).into_plan());
4642
}
4743
if let Some(layout) = layout.as_opt::<Chunked>() {
4844
return Ok(lower_chunked(layout)?.into_plan());

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

Lines changed: 18 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ use crate::OwnedLayoutChildren;
3333
use crate::layouts::chunked::ChunkedLayout;
3434
use crate::layouts::flat::FlatLayout;
3535
use crate::layouts::struct_::StructLayout;
36-
use crate::plan::Filter;
3736
use crate::plan::PlanRef;
3837
use crate::plan::SegmentScan;
3938
use crate::plan::lower;
@@ -521,42 +520,34 @@ fn aligned_fields_stream_one_struct_per_chunk() -> VortexResult<()> {
521520
assert_view(&expected, &(0..ROWS), &mask, run.arrays)
522521
}
523522

524-
/// A bare segment scan returns every row of its range whatever it is told to care about, and a
525-
/// filter over the same scan returns only the selected rows.
523+
/// A segment scan keeps the rows its split selects, and so does a filter over the same scan,
524+
/// which runs as the same source.
526525
#[rstest]
527526
#[case::every_other(Sel::EveryOther)]
528527
#[case::sparse(Sel::Rows(&[1, 4]))]
529528
#[case::nothing(Sel::None)]
530-
fn bare_scan_is_dense_and_filter_keeps_the_selection(#[case] sel: Sel) -> VortexResult<()> {
529+
fn scan_keeps_the_selection_with_or_without_a_filter(#[case] sel: Sel) -> VortexResult<()> {
531530
let mut store = Store::default();
532531
let values = PrimitiveArray::from_iter(0..ROWS as i32).into_array();
533-
let filtered = lower(&store.flat(&values)?)?;
534-
assert!(filtered.is::<Filter>());
535-
let scan = filtered.child_required(0)?;
532+
let scan = lower(&store.flat(&values)?)?;
536533
assert!(scan.is::<SegmentScan>());
534+
let filtered = crate::plan::FilterPlan::new(scan.clone()).into_plan();
537535

538536
let rows = 3..9;
539537
let mask = sel.mask(6);
540-
let mut ctx = SESSION.create_execution_ctx();
541-
542-
let dense = run(
543-
&store,
544-
&scan,
545-
rows.clone(),
546-
mask.clone(),
547-
delivery(Delivery::Fifo),
548-
)?;
549-
assert_eq!(dense.arrays.len(), 1);
550-
assert_arrays_eq!(dense.arrays[0], values.slice(3..9)?, &mut ctx);
551-
552-
let kept = run(
553-
&store,
554-
&filtered,
555-
rows.clone(),
556-
mask.clone(),
557-
delivery(Delivery::Fifo),
558-
)?;
559-
assert_view(&values, &rows, &mask, kept.arrays)?;
538+
for plan in [&scan, &filtered] {
539+
let kept = run(
540+
&store,
541+
plan,
542+
rows.clone(),
543+
mask.clone(),
544+
delivery(Delivery::Fifo),
545+
)?;
546+
if mask.all_false() {
547+
assert_eq!(reads(&kept.events), 0, "nothing selected reads nothing");
548+
}
549+
assert_view(&values, &rows, &mask, kept.arrays)?;
550+
}
560551
Ok(())
561552
}
562553

‎vortex-layout/src/plan/plans/segment_scan.rs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,8 @@ impl PlanVTable for SegmentScan {
105105
mask: &Mask,
106106
compiler: &mut Compiler<'_>,
107107
) -> VortexResult<Option<Chain>> {
108-
let _ = mask;
109-
// A bare scan produces every row of its range; a filter over it keeps the selected ones.
110-
compiler.scan(plan, rows, None)
108+
// A scan keeps the selected rows itself, as every plan produces only those.
109+
compiler.scan(plan, rows, Some(mask.clone()))
111110
}
112111

113112
fn reach(

0 commit comments

Comments
 (0)