Skip to content

Commit cc7d9f4

Browse files
committed
refactor: build known scan expressions as bound expressions directly
Scan callers that know their projection or filter statically were writing `expr.optimize_recursive(dtype)?.bind(dtype)?`. For these shapes the optimizer pass is a no-op, so the round trip only costs a tree walk and produces a fresh tree identity that defeats the identity-keyed caches the parent PR introduces. Build them with the `bound::*` constructors instead, at the crate doc example, the `vortex` and `vortex-file` tests, and the compress/TPC-H benchmarks. The one exception is `and(gt, lt_eq)` over a single column, which the optimizer folds into a `between`; that site now constructs the `between` directly, which is the form the scan was already receiving. Callers whose expression arrives from outside — Python, DataFusion, scan requests and the fuzz target — still optimize and bind, as do the `vortex-file` tests that deliberately exercise expressions which bind but fail during execution. Add `bound_constructors_match_optimize_then_bind`, which asserts each converted shape equals its `optimize_recursive(..).bind(..)` result so the two cannot drift apart. Signed-off-by: Claude <noreply@anthropic.com>
1 parent 6a8d248 commit cc7d9f4

5 files changed

Lines changed: 168 additions & 98 deletions

File tree

benchmarks/compress-bench/src/vortex.rs

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,7 @@ use futures::StreamExt;
1414
use futures::pin_mut;
1515
use vortex::array::IntoArray;
1616
use vortex::dtype::FieldNames;
17-
use vortex::expr::root;
18-
use vortex::expr::select;
17+
use vortex::expr::bound;
1918
use vortex::file::OpenOptionsSessionExt;
2019
use vortex::file::WriteOptionsSessionExt;
2120
use vortex_arrow::ToArrowType;
@@ -71,9 +70,7 @@ impl Compressor for VortexCompressor {
7170
if let Some(cols) = read_projection(root_columns) {
7271
// Columns are named "0".."num_columns-1"; project the given subset.
7372
let names: FieldNames = cols.iter().map(|i| i.to_string()).collect();
74-
let projection = select(names, root())
75-
.optimize_recursive(&source_dtype)?
76-
.bind(&source_dtype)?;
73+
let projection = bound::select(names, bound::root(source_dtype.clone()));
7774
scan = scan.with_projection(projection);
7875
}
7976
let schema = Arc::new(scan.dtype()?.to_arrow_schema()?);

vortex-array/src/expr/mod.rs

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,9 +225,12 @@ mod tests {
225225
use crate::expr::not;
226226
use crate::expr::not_eq;
227227
use crate::expr::or;
228+
use crate::expr::pack;
228229
use crate::expr::select;
229230
use crate::expr::select_exclude;
230231
use crate::scalar::Scalar;
232+
use crate::scalar_fn::fns::between::BetweenOptions;
233+
use crate::scalar_fn::fns::between::StrictComparison;
231234
use crate::scalar_fn::fns::literal::Literal;
232235

233236
#[test]
@@ -410,4 +413,93 @@ mod tests {
410413
let expression = root();
411414
assert!(!expression.contains::<Literal>().unwrap());
412415
}
416+
417+
/// Scan callers that know their expression statically build it with the `bound::*`
418+
/// constructors instead of `optimize_recursive(..).bind(..)`. Both must agree, otherwise
419+
/// those call sites would silently ship a different expression to the scan.
420+
#[test]
421+
fn bound_constructors_match_optimize_then_bind() -> vortex_error::VortexResult<()> {
422+
let u64_scope = DType::Primitive(PType::U64, Nullability::NonNullable);
423+
let struct_scope = DType::Struct(
424+
StructFields::from_iter([
425+
("name", DType::Utf8(Nullability::Nullable)),
426+
("age", DType::Primitive(PType::I32, Nullability::Nullable)),
427+
]),
428+
Nullability::NonNullable,
429+
);
430+
431+
let cases: Vec<(DType, Expression, BoundExpression)> = vec![
432+
(
433+
u64_scope.clone(),
434+
gt(root(), lit(2u64)),
435+
bound::gt(bound::root(u64_scope), bound::lit(2u64)),
436+
),
437+
(
438+
struct_scope.clone(),
439+
select(["name"], root()),
440+
bound::select(["name"], bound::root(struct_scope.clone())),
441+
),
442+
(
443+
struct_scope.clone(),
444+
pack([("name", col("name"))], Nullability::NonNullable),
445+
bound::pack(
446+
[("name", bound::col("name", struct_scope.clone()))],
447+
Nullability::NonNullable,
448+
),
449+
),
450+
(
451+
struct_scope.clone(),
452+
eq(get_item("name", root()), lit("Joseph")),
453+
bound::eq(
454+
bound::col("name", struct_scope.clone()),
455+
bound::lit("Joseph"),
456+
),
457+
),
458+
(
459+
struct_scope.clone(),
460+
or(
461+
eq(get_item("name", root()), lit("Angela")),
462+
and(
463+
gt_eq(get_item("age", root()), lit(20)),
464+
lt_eq(get_item("age", root()), lit(30)),
465+
),
466+
),
467+
bound::or(
468+
bound::eq(
469+
bound::col("name", struct_scope.clone()),
470+
bound::lit("Angela"),
471+
),
472+
bound::and(
473+
bound::gt_eq(bound::col("age", struct_scope.clone()), bound::lit(20)),
474+
bound::lt_eq(bound::col("age", struct_scope.clone()), bound::lit(30)),
475+
),
476+
),
477+
),
478+
// `and(gt, lt_eq)` over a single column is folded into a `between`, so the direct
479+
// form is the `between` rather than the conjunction it was written as.
480+
(
481+
struct_scope.clone(),
482+
and(
483+
gt(get_item("age", root()), lit(21)),
484+
lt_eq(get_item("age", root()), lit(33)),
485+
),
486+
bound::between(
487+
bound::col("age", struct_scope),
488+
bound::lit(21),
489+
bound::lit(33),
490+
BetweenOptions {
491+
lower_strict: StrictComparison::Strict,
492+
upper_strict: StrictComparison::NonStrict,
493+
},
494+
),
495+
),
496+
];
497+
498+
for (scope, unbound, direct) in cases {
499+
let via_optimizer = unbound.optimize_recursive(&scope)?.bind(&scope)?;
500+
assert_eq!(via_optimizer, direct, "mismatch for {unbound}");
501+
}
502+
503+
Ok(())
504+
}
413505
}

vortex-bench/src/datasets/tpch_l_comment.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,7 @@ use vortex::array::IntoArray;
1313
use vortex::array::arrays::ChunkedArray;
1414
use vortex::array::arrays::StructArray;
1515
use vortex::dtype::Nullability::NonNullable;
16-
use vortex::expr::col;
17-
use vortex::expr::pack;
16+
use vortex::expr::bound;
1817
use vortex::file::OpenOptionsSessionExt;
1918

2019
use crate::Format;
@@ -66,9 +65,10 @@ impl Dataset for TPCHLCommentChunked {
6665

6766
let path = data_dir.join("lineitem.vortex");
6867
let file = SESSION.open_options().open_path(path).await?;
69-
let projection = pack(vec![("l_comment", col("l_comment"))], NonNullable)
70-
.optimize_recursive(file.dtype())?
71-
.bind(file.dtype())?;
68+
let projection = bound::pack(
69+
vec![("l_comment", bound::col("l_comment", file.dtype().clone()))],
70+
NonNullable,
71+
);
7272
let chunks: Vec<_> = file
7373
.scan()?
7474
.with_projection(projection)

0 commit comments

Comments
 (0)