Skip to content

refactor: build known scan expressions as bound expressions directly - #9269

Open
joseph-isaacs wants to merge 1 commit into
developfrom
claude/ji-optimal-bound-exprs
Open

refactor: build known scan expressions as bound expressions directly#9269
joseph-isaacs wants to merge 1 commit into
developfrom
claude/ji-optimal-bound-exprs

Conversation

@joseph-isaacs

@joseph-isaacs joseph-isaacs commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

Follow-up to #9192, which made scan builders take BoundExpression so 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:

let projection = select(["value"], root())
    .optimize_recursive(file.dtype())?
    .bind(file.dtype())?;

For these shapes optimize_recursive is 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:

Site Was Now
vortex/src/lib.rs crate doc gt(root(), lit(2u64)) + optimize + bind bound::gt(bound::root(..), bound::lit(2u64))
vortex/src/lib.rs tests select(["value"], root()) + optimize + bind bound::select(["value"], bound::root(..))
vortex-file/src/tests.rs 15 bind_scan_expr(..) calls + an empty-pack projection direct bound::*; helper deleted
vortex-bench TPC-H l_comment pack([..]) + optimize + bind bound::pack([..])
compress-bench select(names, root()) + optimize + bind bound::select(names, bound::root(..))

One case is not a mechanical swap. The optimizer folds and(gt, lt_eq) over a single column into a between:

(($.age > 21i32) and ($.age <= 33i32))  =>  (21i32 < $.age <= 33i32)

Writing that site as bound::and(..) would have silently changed which expression reaches the scan, so it now constructs the between directly — 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. Three vortex-file tests 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 returning Err on a dtype mismatch, so converting them would delete the coverage.

Test. bound_constructors_match_optimize_then_bind asserts each converted shape equals its optimize_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 develop with #9192 merged in:

  • cargo clippy --all-targets on vortex-array, vortex-file, vortex, vortex-bench — clean
  • cargo test -p vortex-array --lib — 3333 passed
  • cargo test -p vortex-file --lib — 131 passed
  • cargo test -p vortex --lib — 23 passed
  • cargo check -p compress-bench --all-targets — clean
  • cargo +nightly fmt --all

Not run locally: DuckDB crates (proxy blocks the DuckDB source download) and vortex-bench tests (lance-encoding needs protoc). 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 in vortex/src/lib.rs, which now shows the direct bound form; that is the snippet users copy, so it should show the cheaper path.

Base automatically changed from ji/remove-bound-expr-unbind to develop August 7, 2026 11:29
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>
@joseph-isaacs
joseph-isaacs force-pushed the claude/ji-optimal-bound-exprs branch from 21fb5de to cc7d9f4 Compare August 7, 2026 11:36
@codspeed-hq

codspeed-hq Bot commented Aug 7, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 4.4%

⚡ 1 improved benchmark
❌ 2 regressed benchmarks
✅ 1926 untouched benchmarks
⏩ 51 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation chunked_dict_primitive_into_canonical[u32, (1000, 10, 100)] 1.6 ms 1.9 ms -11.41%
Simulation chunked_dict_primitive_into_canonical[u32, (1000, 100, 100)] 1.7 ms 1.9 ms -10.44%
Simulation chunked_varbin_opt_to_varbinview_builder[(10, 100)] 870.4 µs 790.5 µs +10.11%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing claude/ji-optimal-bound-exprs (cc7d9f4) with develop (c1ae775)2

Open in CodSpeed

Footnotes

  1. 51 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on develop (6a8d248) during the generation of this report, so c1ae775 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants