Skip to content

refactor: make scalar expressions arena-backed - #380

Merged
KKould merged 2 commits into
mainfrom
refactor/arena-backed-scalar-expressions
Sep 3, 2026
Merged

refactor: make scalar expressions arena-backed#380
KKould merged 2 commits into
mainfrom
refactor/arena-backed-scalar-expressions

Conversation

@KKould

@KKould KKould commented Sep 2, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue link: N/A

ScalarExpression currently owns nested expression values in some paths while other planner structures use arena references. This mixes ownership models, requires repeated cloning and allocation, complicates expression traversal and serialization, and makes plan formatting allocate intermediate strings.

The ORM layer also converts already-bound expressions back into owned ScalarExpression values before allocating them again. Once those expressions retain their ExprRef identity, column pruning must avoid applying the same relative position remap more than once to a shared expression node in a single remap batch.

What is changed and how it works?

  • Makes scalar-expression children arena-backed through ExprRef across binding, planning, optimization, execution, ORM, and serialization.
  • Adds ExprRef reference serialization and updates column/default-expression serialization accordingly.
  • Keeps column-reference-insensitive expression equality local to the expression module and covers every ScalarExpression variant.
  • Aligns immutable and mutable expression visitors around ExprRef, full-expression callbacks, and an explicit fast short-circuit hook.
  • Reuses a column-pruning-owned visited set for each PositionRemapper batch. PositionRemapper::new clears the set so shared nodes are remapped once per mapping without leaking visited state into the next level.
  • Keeps ORM expressions and window partitions in ExprRef form. Already-bound expressions retain their references; only unbound literals/raw expressions are allocated into the current arena.
  • Refactors EXPLAIN into an arena-aware Display-style adapter. Recursive formatting writes directly to fmt::Formatter, while owned strings remain at API boundaries such as LogicalPlan::explain and expression output names.
  • Preserves semantic table.column and index names in EXPLAIN output instead of exposing internal IDs.
  • Moves correlated parameterized IN/NOT IN and EXISTS/NOT EXISTS plan coverage into sqllogictest and updates existing EXPLAIN baselines to full semantic output.

Code changes

  • Has Rust code change
  • Has CI related scripts change

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

The arena-backed representation changes Rust-facing expression and ORM trait signatures, including generated/manual Model implementations. SQL behavior and semantic EXPLAIN names are preserved.

Note for reviewer

This is a broad mechanical refactor, so the main semantic review points are:

  1. ExprRef ownership and lifetime boundaries in PlanArena/TableArena.
  2. ExprVisitor and ExprVisitorMut traversal and short-circuit behavior.
  3. Column-pruning position remapping for shared expression nodes; the visited set is reused by one pruning application and cleared whenever a new PositionRemapper batch is constructed.
  4. ORM conversion of bound expressions without cloning/reallocating ScalarExpression.
  5. EXPLAIN formatting preserving semantic column/index names and existing owned-string API boundaries.
  6. Expression serialization and default-expression materialization into the persistent table arena.

The only warning observed during local test runs is the pre-existing dead-code warning for execute_mut.

@KKould KKould self-assigned this Sep 2, 2026
@KKould KKould added the enhancement New feature or request label Sep 2, 2026
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.03841% with 239 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.07%. Comparing base (8028dc6) to head (113fb7d).

Files with missing lines Patch % Lines
src/orm/mod.rs 81.73% 57 Missing ⚠️
src/expression/mod.rs 90.45% 36 Missing ⚠️
src/expression/simplify.rs 84.65% 29 Missing ⚠️
src/optimizer/rule/normalization/column_pruning.rs 71.66% 17 Missing ⚠️
src/expression/range_detacher.rs 97.05% 14 Missing ⚠️
src/expression/eq_col.rs 98.37% 13 Missing ⚠️
src/planner/arena.rs 89.88% 9 Missing ⚠️
src/expression/visitor_mut.rs 95.97% 8 Missing ⚠️
...ptimizer/rule/normalization/pushdown_predicates.rs 93.63% 7 Missing ⚠️
src/expression/visitor.rs 96.27% 6 Missing ⚠️
... and 22 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #380      +/-   ##
==========================================
+ Coverage   92.95%   93.07%   +0.11%     
==========================================
  Files         255      257       +2     
  Lines       43770    45289    +1519     
==========================================
+ Hits        40687    42152    +1465     
- Misses       3083     3137      +54     
Flag Coverage Δ
rust 93.07% <95.03%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/binder/analyze.rs 96.00% <100.00%> (ø)
src/binder/copy.rs 71.73% <100.00%> (ø)
src/binder/create_view.rs 100.00% <100.00%> (ø)
src/binder/expr.rs 94.17% <100.00%> (-0.09%) ⬇️
src/binder/update.rs 100.00% <100.00%> (ø)
src/catalog/column.rs 92.45% <100.00%> (-3.26%) ⬇️
src/catalog/table.rs 98.76% <100.00%> (+<0.01%) ⬆️
src/db.rs 91.97% <100.00%> (-1.00%) ⬇️
src/execution/ddl/add_column.rs 97.11% <100.00%> (ø)
src/execution/ddl/create_index.rs 92.39% <100.00%> (ø)
... and 98 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@KKould
KKould merged commit 156bb04 into main Sep 3, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant