Skip to content

Commit c4c1bbf

Browse files
authored
Bind expressions before optimizing (#10002)
Supersedes #9514. Optimization now runs only on `BoundExpression`s: callers bind first, then optimize. This removes the optimizer-owned dtype cache, rules can read dtypes straight off the tree, and it unblocks lambdas/variables, which must be typed before optimization. ## Changes - `BoundExpression::optimize` and `optimize_recursive` replace the `Expression` versions. Traversal and the fixpoint loop are unchanged. - `ScalarFnVTable::simplify` now takes a `&BoundExpression`, and `simplify_untyped` is merged into it. `SimplifyCtx` and `SimplifyCache` are removed. - `ExpressionReduceNode` keeps its name and `reduce_expression` dispatch, but now borrows a bound tree. - Rules ported: binary, case_when, cast, fill_null, mask, select, zip, and `find_between`. `GetItem::simplify_untyped` is removed because it duplicated `GetItem::reduce`. If the rebuilt `Between` fails to type-check, `find_between` now skips the rewrite instead of producing a tree that fails to bind. - Callers change from `expr.optimize_recursive(dtype)?.bind(dtype)` to `expr.bind(dtype)?.optimize_recursive()`. - The unbound `expr::transform::partition` is deleted. It was the last user of the unbound optimizer and has no callers; layouts use `bound_partition`. #9993 deletes it too, so whichever PR lands second will have a trivial conflict. ## API changes - `Expression::optimize(scope)` / `optimize_recursive(scope)` become `BoundExpression::optimize()` / `optimize_recursive()`. - `ScalarFnVTable::simplify(options, &BoundExpression)` replaces `simplify(options, &Expression, &dyn SimplifyCtx)` and `simplify_untyped`. - `ExpressionReduceNode::new(&BoundExpression)` drops the scope argument. - `partition`, `partition_annotations` and `PartitionedExpr` are removed. ## Behavior I checked this with a differential corpus of 30,018 generated expressions targeting every rule, comparing develop's `optimize + bind` against the new `bind + optimize` and evaluating before/after on data: - 28,878 produce identical output. - 131 panicked on develop (`Mask` simplify called `vortex_expect` on a null mask literal) and now fail cleanly at bind time. - 941 have ill-typed inputs that develop accepted only because a rule removed the ill-typed subtree before binding. They now error. - 68 differ because of a develop bug. `SimplifyCache` keys dtypes by `Expression`, and `Scalar` equality ignores nullability, so develop reused the wrong cached dtype and dropped `fill_null`s it should have kept. No well-typed expression regressed. Existing bugs found, not addressed here: - The `CASE WHEN`→`fill_null` rewrite is unexecutable for utf8. - `select([], nullable_struct)` drops the struct's validity. - `CaseWhen` execution can return a result that is non-nullable when its dtype is nullable. - `and`/`or`/`zip` literal rules can change the result's nullability. ## Performance The optimizer benchmarks bind outside the timed loop and measure only `optimize_recursive`, the same work develop's benchmarks time. Local medians: | case | develop | this PR | |---|---|---| | `or_chain` 200 | 17.5 µs | 18.6 µs | | `balanced_or` 64–256 ids | 6.2–23.0 µs | 6.1–23.5 µs | | `balanced_or` 1 id | 0.79 µs | 0.94 µs | | `in_list` | ~0.77 µs | ~0.95 µs | Small predicates pay a constant ~0.15–0.2 µs in the optimizer, almost all of it in `find_between`, which now type-checks the `and`/`Between` nodes it rebuilds (0.56 µs vs 0.34 µs). Across the full `bind + optimize` pipeline that callers run, the gap is ~0.3–0.45 µs, because binding also types the comparisons `find_between` later replaces. Closing it would need an unchecked `BoundExpression` constructor. Open question. ## Testing - `cargo nextest run -p vortex-array -p vortex-layout -p vortex-file -p vortex -p vortex-datafusion -p vortex-duckdb` - `cargo test --doc -p vortex -p vortex-array` - `cargo clippy --all-targets --all-features -- -D warnings` on all changed crates and `fuzz/file_io` - `cargo check --workspace --all-targets` Not run: Python tests. `vortex-cuda-ffi` was not type-checked because it is compiled out on macOS. Signed-off-by: Matt Katz <mhkatz97@gmail.com>
1 parent 0fea4a3 commit c4c1bbf

33 files changed

Lines changed: 365 additions & 831 deletions

File tree

‎benchmarks/compress-bench/src/vortex.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ impl Compressor for VortexCompressor {
7777
// Columns are named "0".."num_columns-1"; project the given subset.
7878
let names: FieldNames = cols.iter().map(|i| i.to_string()).collect();
7979
let projection = select(names, root())
80-
.optimize_recursive(&source_dtype)?
81-
.bind(&source_dtype)?;
80+
.bind(&source_dtype)?
81+
.optimize_recursive()?;
8282
scan = scan.with_projection(projection);
8383
}
8484
let schema = Arc::new(SESSION.arrow().to_arrow_schema(&scan.dtype()?)?);

‎docs/developer-guide/internals/session.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,8 @@ session.write_options()
7979

8080
// Scanning a layout
8181
let filter = expr
82-
.optimize_recursive(layout_reader.dtype())?
83-
.bind(layout_reader.dtype())?;
82+
.bind(layout_reader.dtype())?
83+
.optimize_recursive()?;
8484
ScanBuilder::new(session.clone(), layout_reader)
8585
.with_filter(filter)
8686
.into_array_stream()?;

‎fuzz/fuzz_targets/file_io.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,14 +83,14 @@ fuzz_target!(|fuzz: FuzzFileAction| -> Corpus {
8383
.vortex_expect("open_buffer should succeed in fuzz test");
8484
let projection = projection_expr
8585
.unwrap_or_else(root)
86-
.optimize_recursive(file.dtype())
87-
.and_then(|expr| expr.bind(file.dtype()))
86+
.bind(file.dtype())
87+
.and_then(|expr| expr.optimize_recursive())
8888
.vortex_expect("projection should bind in fuzz test");
8989
let filter = filter_expr
9090
.map(|filter| {
9191
filter
92-
.optimize_recursive(file.dtype())
93-
.and_then(|expr| expr.bind(file.dtype()))
92+
.bind(file.dtype())
93+
.and_then(|expr| expr.optimize_recursive())
9494
})
9595
.transpose()
9696
.vortex_expect("filter should bind in fuzz test");

‎vortex-array/benches/expr/optimize_bench.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ fn build_or_chain(n: usize) -> Expression {
4343

4444
#[divan::bench(args = [200])]
4545
fn optimize_or_chain(bencher: Bencher, n: usize) {
46-
let expr = build_or_chain(n);
4746
let scope = struct_scope();
48-
bencher.bench(|| expr.optimize_recursive(&scope).unwrap());
47+
let expr = build_or_chain(n).bind(&scope).unwrap();
48+
bencher.bench(|| expr.optimize_recursive().unwrap());
4949
}

‎vortex-array/benches/expr/optimize_predicate.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// SPDX-License-Identifier: Apache-2.0
22
// SPDX-FileCopyrightText: Copyright the Vortex contributors
33

4-
//! Benchmarks `Expression::optimize_recursive` on lookup-style pushdown predicates:
4+
//! Benchmarks `BoundExpression::optimize_recursive` on lookup-style pushdown predicates:
55
//! an id membership test (either `list_contains` or a balanced OR of equalities) conjoined
66
//! with timestamp range bounds and kind filters.
77
@@ -207,7 +207,7 @@ fn lookup_predicate(predicate_case: PredicateCase) -> Expression {
207207
#[divan::bench(args = PREDICATE_CASES)]
208208
fn optimize_lookup_predicate(bencher: Bencher, predicate_case: &PredicateCase) {
209209
let scope = scope();
210-
let predicate = lookup_predicate(*predicate_case);
210+
let predicate = lookup_predicate(*predicate_case).bind(&scope).unwrap();
211211

212-
bencher.bench(|| black_box(predicate.optimize_recursive(&scope)));
212+
bencher.bench(|| black_box(predicate.optimize_recursive()));
213213
}

‎vortex-array/src/expr/optimize.rs‎

Lines changed: 29 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -1,46 +1,29 @@
11
// SPDX-License-Identifier: Apache-2.0
22
// SPDX-FileCopyrightText: Copyright the Vortex contributors
33

4-
use std::cell::RefCell;
5-
6-
use itertools::Itertools;
74
use vortex_error::VortexResult;
8-
use vortex_error::vortex_err;
9-
use vortex_utils::aliases::hash_map::HashMap;
105

11-
use crate::dtype::DType;
12-
use crate::expr::Expression;
6+
use crate::expr::BoundExpression;
137
use crate::expr::transform::match_between::find_between;
148
use crate::scalar_fn::ExpressionReduceNode;
15-
use crate::scalar_fn::SimplifyCtx;
169

17-
impl Expression {
10+
impl BoundExpression {
1811
/// Optimize the root expression node only, iterating to convergence.
1912
///
2013
/// This applies optimization rules repeatedly until no more changes occur:
21-
/// 1. `simplify_untyped` - type-independent simplifications
22-
/// 2. `simplify` - type-aware simplifications
23-
/// 3. `reduce` - abstract reduction rules via `ReduceNode`
24-
pub fn optimize(&self, scope: &DType) -> VortexResult<Expression> {
25-
let cache = SimplifyCache::new(scope);
26-
Ok(self.try_optimize(&cache)?.unwrap_or_else(|| self.clone()))
14+
/// 1. `simplify` - scalar-function simplifications over the bound node
15+
/// 2. `reduce` - abstract reduction rules via `ReduceNode`
16+
pub fn optimize(&self) -> VortexResult<BoundExpression> {
17+
Ok(self.try_optimize()?.unwrap_or_else(|| self.clone()))
2718
}
2819

29-
/// Apply this node's own untyped simplification rule, if it has one.
20+
/// Apply this node's own simplification rule, if it has one.
3021
///
3122
/// Non-scalar nodes carry no rules, so they never simplify.
32-
fn simplify_untyped_node(&self) -> VortexResult<Option<Expression>> {
23+
fn simplify_node(&self) -> VortexResult<Option<BoundExpression>> {
3324
match self {
34-
Expression::Scalar { scalar_fn, .. } => scalar_fn.simplify_untyped(self),
35-
Expression::Root => Ok(None),
36-
}
37-
}
38-
39-
/// Apply this node's own type-aware simplification rule, if it has one.
40-
fn simplify_node(&self, ctx: &dyn SimplifyCtx) -> VortexResult<Option<Expression>> {
41-
match self {
42-
Expression::Scalar { scalar_fn, .. } => scalar_fn.simplify(self, ctx),
43-
Expression::Root => Ok(None),
25+
BoundExpression::Scalar { scalar_fn, .. } => scalar_fn.simplify(self),
26+
BoundExpression::Root { .. } => Ok(None),
4427
}
4528
}
4629

@@ -50,16 +33,16 @@ impl Expression {
5033
node: &ExpressionReduceNode<'a>,
5134
) -> VortexResult<Option<ExpressionReduceNode<'a>>> {
5235
match self {
53-
Expression::Scalar { scalar_fn, .. } => scalar_fn.reduce_expression(node),
54-
Expression::Root => Ok(None),
36+
BoundExpression::Scalar { scalar_fn, .. } => scalar_fn.reduce_expression(node),
37+
BoundExpression::Root { .. } => Ok(None),
5538
}
5639
}
5740

5841
/// Try to optimize the root expression node only, returning None if no optimizations applied.
59-
fn try_optimize(&self, cache: &SimplifyCache<'_>) -> VortexResult<Option<Expression>> {
42+
fn try_optimize(&self) -> VortexResult<Option<BoundExpression>> {
6043
// Copy-on-write: `current` stays None until a rule fires, so unchanged nodes (the common
6144
// case) are never cloned.
62-
let mut current: Option<Expression> = None;
45+
let mut current: Option<BoundExpression> = None;
6346
let mut loop_counter = 0;
6447

6548
loop {
@@ -73,25 +56,16 @@ impl Expression {
7356
let expr = current.as_ref().unwrap_or(self);
7457
let mut changed = false;
7558

76-
// Try simplify_untyped
77-
if let Some(simplified) = expr.simplify_untyped_node()? {
59+
if let Some(simplified) = expr.simplify_node()? {
7860
current = Some(simplified);
7961
changed = true;
8062
}
8163

82-
// Try simplify (typed)
83-
let expr = current.as_ref().unwrap_or(self);
84-
if let Some(simplified) = expr.simplify_node(cache)? {
85-
current = Some(simplified);
86-
changed = true;
87-
}
88-
89-
// Try reduce via ReduceNode. The node borrows the expression and scope, so
90-
// constructing it is free; the block scopes the borrows so `current` can be updated.
64+
// Try reduce via ReduceNode. The node borrows the expression, so constructing it is
65+
// free; the block scopes the borrow so `current` can be updated.
9166
let reduced = {
9267
let expr = current.as_ref().unwrap_or(self);
93-
let reduce_node = ExpressionReduceNode::new(expr, cache.scope);
94-
expr.reduce_node(&reduce_node)?
68+
expr.reduce_node(&ExpressionReduceNode::new(expr))?
9569
.map(ExpressionReduceNode::into_expression)
9670
};
9771
if let Some(reduced_expr) = reduced {
@@ -110,38 +84,33 @@ impl Expression {
11084
/// Optimize the entire expression tree recursively.
11185
///
11286
/// Optimizes children first (bottom-up), then optimizes the root.
113-
pub fn optimize_recursive(&self, scope: &DType) -> VortexResult<Expression> {
87+
pub fn optimize_recursive(&self) -> VortexResult<BoundExpression> {
11488
Ok(self
115-
.clone()
116-
.try_optimize_recursive(scope)?
89+
.try_optimize_recursive()?
11790
.unwrap_or_else(|| self.clone()))
11891
}
11992

12093
/// Try to optimize the entire expression tree recursively.
121-
pub fn try_optimize_recursive(&self, scope: &DType) -> VortexResult<Option<Expression>> {
122-
let cache = SimplifyCache::new(scope);
123-
let result = self.try_optimize_recursive_inner(&cache)?;
94+
pub fn try_optimize_recursive(&self) -> VortexResult<Option<BoundExpression>> {
95+
let result = self.try_optimize_recursive_inner()?;
12496

12597
// Apply the between optimization once at the top level only.
12698
// TODO(ngates): remove the "between" optimization, or rewrite it to not always convert
12799
// to CNF?
128100
Ok(Some(find_between(result.unwrap_or_else(|| self.clone()))))
129101
}
130102

131-
fn try_optimize_recursive_inner(
132-
&self,
133-
cache: &SimplifyCache<'_>,
134-
) -> VortexResult<Option<Expression>> {
103+
fn try_optimize_recursive_inner(&self) -> VortexResult<Option<BoundExpression>> {
135104
// First optimize the root
136-
let mut current = self.try_optimize(cache)?;
105+
let mut current = self.try_optimize()?;
137106

138107
// Then recursively optimize children. The new children vector is only allocated once a
139108
// child actually changes, so fully-optimized subtrees cost no allocations.
140109
let expr = current.as_ref().unwrap_or(self);
141110
let children = expr.children();
142-
let mut new_children: Option<Vec<Expression>> = None;
111+
let mut new_children: Option<Vec<BoundExpression>> = None;
143112
for (idx, child) in children.iter().enumerate() {
144-
if let Some(optimized) = child.try_optimize_recursive_inner(cache)? {
113+
if let Some(optimized) = child.try_optimize_recursive_inner()? {
145114
new_children
146115
.get_or_insert_with(|| children[..idx].to_vec())
147116
.push(optimized);
@@ -154,56 +123,13 @@ impl Expression {
154123
let updated = expr.clone().with_children(new_children)?;
155124

156125
// After updating children, try to optimize root again
157-
current = Some(updated.try_optimize(cache)?.unwrap_or(updated));
126+
current = Some(updated.try_optimize()?.unwrap_or(updated));
158127
}
159128

160129
Ok(current)
161130
}
162131
}
163132

164-
struct SimplifyCache<'a> {
165-
scope: &'a DType,
166-
dtype_cache: RefCell<HashMap<Expression, DType>>,
167-
}
168-
169-
impl<'a> SimplifyCache<'a> {
170-
fn new(scope: &'a DType) -> Self {
171-
Self {
172-
scope,
173-
dtype_cache: RefCell::new(HashMap::new()),
174-
}
175-
}
176-
}
177-
178-
impl SimplifyCtx for SimplifyCache<'_> {
179-
fn return_dtype(&self, expr: &Expression) -> VortexResult<DType> {
180-
// If the expression is "root", return the scope dtype
181-
if expr.is_root() {
182-
return Ok(self.scope.clone());
183-
}
184-
185-
if let Some(dtype) = self.dtype_cache.borrow().get(expr) {
186-
return Ok(dtype.clone());
187-
}
188-
189-
// Otherwise, compute dtype from children
190-
let input_dtypes: Vec<_> = expr
191-
.children()
192-
.iter()
193-
.map(|c| self.return_dtype(c))
194-
.try_collect()?;
195-
let dtype = expr
196-
.as_scalar()
197-
.ok_or_else(|| vortex_err!("cannot type a non-scalar expression: {expr}"))?
198-
.return_dtype(&input_dtypes)?;
199-
self.dtype_cache
200-
.borrow_mut()
201-
.insert(expr.clone(), dtype.clone());
202-
203-
Ok(dtype)
204-
}
205-
}
206-
207133
#[cfg(test)]
208134
mod tests {
209135
use vortex_error::VortexResult;
@@ -236,7 +162,7 @@ mod tests {
236162
),
237163
Nullability::NonNullable,
238164
);
239-
let optimized = expr.optimize_recursive(&scope)?;
165+
let optimized = expr.bind(&scope)?.optimize_recursive()?;
240166

241167
let s = optimized.to_string();
242168
assert!(s.contains("$.x"), "expected $.x in {s}");
@@ -261,7 +187,7 @@ mod tests {
261187
),
262188
Nullability::NonNullable,
263189
);
264-
let optimized = expr.optimize_recursive(&scope)?;
190+
let optimized = expr.bind(&scope)?.optimize_recursive()?;
265191

266192
// Prune rules pattern-match a bare Literal on the comparison RHS; a cast wrapper
267193
// silently disables pruning.

0 commit comments

Comments
 (0)