Make BoundExpression reference-counted and identity-keyed - #9492
Draft
joseph-isaacs wants to merge 2 commits into
Draft
Make BoundExpression reference-counted and identity-keyed#9492joseph-isaacs wants to merge 2 commits into
joseph-isaacs wants to merge 2 commits into
Conversation
`BoundExpression` was passed around by value, with its children behind a single `Arc<Vec<BoundExpression>>` so that clones stayed cheap. That made node identity a property of the child vector rather than of the node, and it meant every node clone still copied a `DType` and a `ScalarFnRef`. Introduce `BoundExpressionRef = Arc<BoundExpression>` as the currency of the bound tree, and hold children as `Box<[BoundExpressionRef]>`. Cloning a subtree is now a refcount bump on the node itself, and rebuilding a node keeps the untouched children in place. Threaded through every crate that builds, walks, or caches bound expressions: constructors, `bound::*` builders, traversal, analysis, partitioning, stats rewrites, layouts, scans, pruning, and CUDA. Notable details: - `Node` is implemented for `BoundExpressionRef` rather than for the enum, so traversals hand out shared handles and identity survives a walk. - `with_children`, `falsify`, and `satisfy` take `self: BoundExpressionRef`. - `ExactBoundExpr` gains an `Arc::ptr_eq` fast path and compares child handles elementwise, preserving its previous identity semantics. - The iterative `Drop` walks shared handles, descending only into subtrees it is the last owner of. Covered by a new deep-tree regression test that overflows the stack without it. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
joseph-isaacs
added
action/bench-all
…cty3v Reconciles #9479, which dropped the dtype fast path from `with_children` and routed both constructors through a shared inner one. Kept that shape: `try_new_boxed` takes the already-collected `Box<[BoundExpressionRef]>`, so `with_children` does not re-collect. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
joseph-isaacs
added
the
action/bench-all
github-actions
Bot
removed
the
action/bench-all
Merging this PR will regress 1 benchmark
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | words_gather_scalar[65536] |
8.3 µs | 9.4 µs | -12.03% |
| ⚡ | Simulation | filter_random_by_mostly_true[250000] |
237.2 µs | 207.1 µs | +14.52% |
| ⚡ | Simulation | filter_powerlaw_by_mostly_true[250000] |
237.2 µs | 207.2 µs | +14.48% |
| ⚡ | Simulation | filter_random_by_random[250000] |
212.9 µs | 189.2 µs | +12.54% |
| ⚡ | Simulation | filter_powerlaw_by_random[250000] |
212.9 µs | 189.2 µs | +12.54% |
| ⚡ | Simulation | density_sweep_random[0.9] |
115.6 µs | 103.6 µs | +11.63% |
| ⚡ | Simulation | filter_random_by_mostly_true[100000] |
116.4 µs | 104.3 µs | +11.58% |
| ⚡ | Simulation | density_sweep_random[0.95] |
116.2 µs | 104.3 µs | +11.49% |
| ⚡ | Simulation | filter_powerlaw_by_mostly_true[100000] |
116.9 µs | 105 µs | +11.39% |
| ⚡ | Simulation | density_sweep_dense_runs[0.9] |
115.4 µs | 103.6 µs | +11.34% |
| ⚡ | Simulation | density_sweep_dense_runs[0.95] |
116.3 µs | 104.5 µs | +11.27% |
| ⚡ | Simulation | chunked_varbinview_opt_into_canonical[(10, 100)] |
669.6 µs | 604.4 µs | +10.78% |
| ⚡ | Simulation | filter_random_by_power_law[250000] |
197.2 µs | 178.1 µs | +10.73% |
| ⚡ | Simulation | filter_powerlaw_by_power_law[250000] |
197.4 µs | 178.3 µs | +10.69% |
| ⚡ | Simulation | compress_fsst[(500, 64, 4)] |
517 µs | 469.4 µs | +10.13% |
| ⚡ | Simulation | filter_random_by_mostly_false[250000] |
190.2 µs | 172.8 µs | +10.05% |
| ⚡ | Simulation | filter_powerlaw_by_mostly_false[250000] |
190.3 µs | 173 µs | +10.02% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/boundexpression-arc-box-refactor-fcty3v (09cf918) with develop (a4af10a)2
Footnotes
-
474 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. ↩
-
No successful run was found on
develop(6800829) during the generation of this report, so a4af10a was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
Contributor
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Contributor
Benchmarks: String Encoding 📖vortex / vortex-file-compressed / ms (1.011x ➖, 0↑ 0↓)
vortex / vortex-file-compressed / % (1.000x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: PolarSignals Profiling 📖Vortex (geomean): 1.037x ➖ datafusion / vortex-file-compressed / ns (1.037x ➖, 1↑ 2↓)
No file size changes detected. |
Contributor
Benchmarks: Random Access 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
random-access / vortex-file-compressed / ns (0.983x ➖, 0↑ 0↓)
random-access / parquet / ns (0.997x ➖, 0↑ 0↓)
random-access / lance / ns (0.993x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Clickbench on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.981x ➖, 3↑ 2↓)
datafusion / vortex-compact / ns (1.001x ➖, 1↑ 1↓)
datafusion / parquet / ns (1.008x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.984x ➖, 3↑ 1↓)
duckdb / vortex-compact / ns (0.974x ➖, 5↑ 1↓)
duckdb / parquet / ns (0.996x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: FineWeb NVMe 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.003x ➖, 0↑ 0↓)
datafusion / vortex-compact / ns (0.985x ➖, 1↑ 1↓)
datafusion / parquet / ns (1.004x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.091x ➖, 0↑ 3↓)
duckdb / vortex-compact / ns (1.027x ➖, 0↑ 2↓)
duckdb / parquet / ns (0.998x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: Clickbench Sorted on NVME 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.985x ➖, 1↑ 1↓)
datafusion / vortex-compact / ns (0.979x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.023x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed / ns (1.015x ➖, 1↑ 1↓)
duckdb / vortex-compact / ns (0.972x ➖, 2↑ 2↓)
duckdb / parquet / ns (1.002x ➖, 0↑ 1↓)
File Size Changes (200 files changed, +0.0% overall, 101↑ 99↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=1 on S3 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.024x ➖, 0↑ 0↓)
datafusion / vortex-compact / ns (1.075x ➖, 0↑ 3↓)
datafusion / parquet / ns (0.993x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.085x ➖, 1↑ 3↓)
duckdb / vortex-compact / ns (1.028x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.944x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Statistical and Population Genetics 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
duckdb / vortex-file-compressed / ns (0.998x ➖, 1↑ 1↓)
duckdb / vortex-compact / ns (1.025x ➖, 3↑ 3↓)
duckdb / parquet / ns (1.000x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-DS SF=1 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.998x ➖, 0↑ 1↓)
datafusion / vortex-compact / ns (0.993x ➖, 3↑ 1↓)
datafusion / parquet / ns (0.997x ➖, 1↑ 1↓)
duckdb / vortex-file-compressed / ns (0.993x ➖, 7↑ 5↓)
duckdb / vortex-compact / ns (0.988x ➖, 4↑ 3↓)
duckdb / parquet / ns (1.011x ➖, 5↑ 8↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-H SF=10 on S3 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-compact / ns (0.899x ➖, 1↑ 0↓)
datafusion / parquet / ns (1.118x ➖, 1↑ 5↓)
duckdb / vortex-compact / ns (0.928x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.955x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: TPC-H SF=1 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.998x ➖, 0↑ 0↓)
datafusion / vortex-compact / ns (1.002x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.993x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed / ns (1.001x ➖, 0↑ 1↓)
duckdb / vortex-compact / ns (1.005x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.998x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: Compression 📖vortex / vortex-file-compressed / ns (0.978x ➖, 2↑ 0↓)
vortex / vortex-file-compressed / bytes (0.999x ➖, 0↑ 0↓)
vortex / vortex-file-compressed / ratio (0.985x ➖, 2↑ 0↓)
vortex / parquet / ns (1.000x ➖, 0↑ 0↓)
vortex / parquet / bytes (1.000x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: TPC-H SF=10 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.003x ➖, 0↑ 0↓)
datafusion / vortex-compact / ns (1.010x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.009x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.987x ➖, 0↑ 0↓)
duckdb / vortex-compact / ns (1.001x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.010x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: Appian on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-compact / ns (1.000x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.989x ➖, 0↑ 0↓)
duckdb / vortex-compact / ns (1.005x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.010x ➖, 0↑ 0↓)
File Size Changes (10 files changed, -63.8% overall, 0↑ 10↓)
Totals:
|
Contributor
Benchmarks: FineWeb S3 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.039x ➖, 0↑ 1↓)
datafusion / vortex-compact / ns (0.829x ➖, 2↑ 0↓)
datafusion / parquet / ns (1.026x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.974x ➖, 0↑ 0↓)
duckdb / vortex-compact / ns (0.986x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.810x ➖, 1↑ 0↓)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
This change refactors
BoundExpressionto use reference counting (Arc) throughout the codebase, making bound expression trees immutable and cheaply shareable. This is a foundational improvement that enables:ExactBoundExprwrapper now uses pointer equality to detect when two expressions are the same node, avoiding expensive structural comparisons and tree walks during cache lookups.with_childrenmethod can now rebuild nodes while keeping untouched subtrees in place via shared references.This is part of a broader effort to improve expression handling performance in the query optimizer and stats rewrite system.
What changes are included in this PR?
Core Changes
BoundExpressionReftype alias: Introducedpub type BoundExpressionRef = Arc<BoundExpression>as the standard way to pass bound expressions around.BoundExpression::new_root()andBoundExpression::try_new()now returnBoundExpressionRefinstead of bareBoundExpression.Arc<Vec<BoundExpression>>toBox<[BoundExpressionRef]>for more efficient storage of already-shared child references.ExactBoundExpridentity semantics: Updated to compare pointer equality first, then fall back to structural comparison of child pointers. This makes identity-keyed caches fast in the common case where the same node is looked up multiple times.Nodetrait implementation forBoundExpressionRefto work with the new structure.API Updates
All public functions that work with bound expressions now use
BoundExpressionRef:exprs.rs(bound_root,bound_lit,bound_col,bound_get_item,bound_binary, etc.)Implementation Details
.clone()calls onBoundExpressionwithArc::clone()where appropriate to make reference counting explicit.Arcpointers when needed (e.g.,&*self.0inExactBoundExpr).children()method that returns a slice ofBoundExpressionRef.What APIs are changed? Are there any user-facing changes?
Public API changes:
BoundExpression::new_root()now returnsBoundExpressionRefinstead ofBoundExpressionBoundExpression::try_new()now returnsBoundExpressionRefinstead ofBoundExpressionBoundExpression::with_children()now takes and returnsBoundExpressionRefexprs.rsnow returnBoundExpressionRefExactBoundExprnow wrapsBoundExpressionRefinstead ofBoundExpressionImpact:
BoundExpressionvalues. However, the change is straightforward: wrap results inArc::new()or use the provided builder functions which now handle this automatically.Arc::clone()instead of.clone()where appropriate.https://claude.ai/code/session_01TLcv1bEzNV3m5h2XfvjBGo