refactor: build known scan expressions as bound expressions directly - #9269
refactor: build known scan expressions as bound expressions directly#9269joseph-isaacs wants to merge 1 commit into
Conversation
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>
21fb5de to
cc7d9f4
Compare
Merging this PR will degrade performance by 4.4%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
Rationale for this change
Follow-up to #9192, which made scan builders take
BoundExpressionso one bound tree survives planning and execution — rebinding a structurally identical expression mints a new tree identity and defeats the identity-keyed caches.Callers that know their expression statically were still writing:
For these shapes
optimize_recursiveis a no-op — I verified this against the optimizer for every shape converted here. So the round trip buys nothing and costs a full tree walk plus a fresh tree identity. The expression is known at the call site, so the optimal bound form can just be written down.What changes are included in this PR?
Converted the statically-known sites to the
bound::*constructors added by #9192:vortex/src/lib.rscrate docgt(root(), lit(2u64))+ optimize + bindbound::gt(bound::root(..), bound::lit(2u64))vortex/src/lib.rstestsselect(["value"], root())+ optimize + bindbound::select(["value"], bound::root(..))vortex-file/src/tests.rsbind_scan_expr(..)calls + an empty-pack projectionbound::*; helper deletedvortex-benchTPC-Hl_commentpack([..])+ optimize + bindbound::pack([..])compress-benchselect(names, root())+ optimize + bindbound::select(names, bound::root(..))One case is not a mechanical swap. The optimizer folds
and(gt, lt_eq)over a single column into abetween:Writing that site as
bound::and(..)would have silently changed which expression reaches the scan, so it now constructs thebetweendirectly — the form the scan was already receiving.Deliberately left alone. Callers whose expression arrives from outside still optimize and bind, because there the optimizer does real work:
vortex-python,vortex-datafusion,ScanBuilder/multi-scan request handling, and the fuzz target. Threevortex-filetests also keep the fallible path — they exercise expressions that bind but fail during execution, which is exactly the fallible API's contract;bound::*panics rather than returningErron a dtype mismatch, so converting them would delete the coverage.Test.
bound_constructors_match_optimize_then_bindasserts each converted shape equals itsoptimize_recursive(..).bind(..)result, so a future optimizer rule that starts rewriting one of these shapes fails the test instead of silently diverging from the call sites.Checks
Re-run after rebasing onto
developwith #9192 merged in:cargo clippy --all-targetsonvortex-array,vortex-file,vortex,vortex-bench— cleancargo test -p vortex-array --lib— 3333 passedcargo test -p vortex-file --lib— 131 passedcargo test -p vortex --lib— 23 passedcargo check -p compress-bench --all-targets— cleancargo +nightly fmt --allNot run locally: DuckDB crates (proxy blocks the DuckDB source download) and
vortex-benchtests (lance-encodingneedsprotoc). The crate-level doc example is not collected as a doctest in my environment either before or after this change, so I extracted and compiled it separately to confirm it still builds — that may be worth a look separately.What APIs are changed? Are there any user-facing changes?
No API changes —
bound::*already landed in #9192. The only user-facing change is the crate-level doc example invortex/src/lib.rs, which now shows the direct bound form; that is the snippet users copy, so it should show the cheaper path.