perf(logical-plan): box CreateExternalTable / CreateFunction in DdlStatement (-45% LogicalPlan size)#22733
Merged
Conversation
…atement These two variants dominate the LogicalPlan enum size: CreateExternalTable is 312 bytes and CreateFunction is 288 bytes, while every non-DDL variant is at most 176 bytes (Join). The DdlStatement variant therefore forces LogicalPlan to 320 bytes, even though no SELECT-path workload ever materializes a CreateExternalTable / CreateFunction. Boxing both shrinks LogicalPlan from 320 bytes to 176 bytes (-45%) by making Join the new size-determining variant (the enum discriminant fits inside Join's alignment padding). Every mem::take / mem::swap / Arc<LogicalPlan> store on the planning hot path now moves a smaller payload. Local sql_planner --quick: optimizer_tpch_all: 8.61ms -> 8.18ms (-5.0%) optimizer_tpcds_all: 168.0ms -> 163.5ms (-2.7%) Profiling shows ~13% of sql_planner CPU in libsystem_platform.dylib (memcpy / memmove) -- this change directly reduces that pool. A test_size_of_logical_plan unit test pins the new size and asserts DdlStatement stays smaller than Join, so future variant growth that would re-balloon the enum trips the test rather than silently regressing the planning hot path. 711 optimizer + 218 expr + 86 sql + 7 proto unit tests pass. SLT create_function / create_external_table / ddl pass. clippy clean.
github-actions
Bot
added
sql
Contributor
Author
|
run benchmark sql_planner |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/box-large-ddl-variants (8b02e90) to 533ef35 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces LogicalPlan’s stack size by boxing the two oversized DdlStatement variants (CreateExternalTable and CreateFunction), preventing DDL-only payloads from dominating the enum size on the SELECT-query hot path.
Changes:
- Box
DdlStatement::CreateExternalTableandDdlStatement::CreateFunctionand update all construction / match / (de)serialization sites accordingly. - Add a unit test to pin
size_of::<LogicalPlan>() == 176and assertDdlStatementstays smaller thanJoin. - Adjust FFI/proto boundaries to move values in/out of the new
Box<…>wrappers.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| datafusion/sql/src/statement.rs | Wraps DDL plan construction sites with Box::new(...) for the newly-boxed variants. |
| datafusion/proto/src/logical_plan/mod.rs | Updates protobuf ↔ logical-plan conversion to box/unbox CreateExternalTable and adjusts field destructuring accordingly. |
| datafusion/ffi/src/table_provider_factory.rs | Updates FFI plan extraction/construction to unbox/box CreateExternalTable at the boundary. |
| datafusion/expr/src/logical_plan/plan.rs | Adds test_size_of_logical_plan to prevent future LogicalPlan size regressions. |
| datafusion/expr/src/logical_plan/ddl.rs | Changes DdlStatement variant payloads to Box<...> and updates schema/display accessors. |
| datafusion/core/src/execution/context/mod.rs | Unboxes CreateFunction when executing DDL so create_function continues to take an owned CreateFunction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
cc @alamb Benchmark results from #22733 (comment) are in — broad, consistent wins on heavy benchmarks, no real regressions. Optimizer phase (where the change directly applies):
Physical-plan phase (also walks
Regressions: none of material size. A handful of sub-200µs benches sit at +1% inside the noise band; the bench harness shows no statistically distinguishable slowdown anywhere. Profiling motivation held up: ~13% of |
alamb
approved these changes
Jun 4, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Makes sense to me and is consistent with our other structures. Thank you @zhuqi-lucas
Contributor
|
I think it is worth a short note in the upgrade guide too |
Contributor
Author
Thank you @alamb for review , i will update the upgrade guide too! |
Contributor
Author
github-actions
Bot
added
the
documentation
Contributor
Author
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
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.
Which issue does this PR close?
Closes #22732.
Rationale for this change
LogicalPlanis 320 bytes today, sized by theDdl(DdlStatement)variant — which is itself 320 bytes because it carriesCreateExternalTable(312 bytes) andCreateFunction(288 bytes). Every non-DDL variant is at most 176 bytes (Join). Every SELECT query pays the full 320-byte payload on everymem::take/mem::swap/Arc<LogicalPlan>write during planning, even though it never instantiates a DDL node.Profiling
sql_planner(samply, macOS aarch64) showed ~13% of CPU inlibsystem_platform.dylib(memcpy / memmove). Shrinking the most frequently-moved type directly attacks that pool.What changes are included in this PR?
Box the two oversized DDL variants:
After this change:
DdlStatementdrops from 320 → ~152 bytes (max variant is nowCreateIndexat 144).LogicalPlandrops from 320 → 176 bytes (–45%) — the enum discriminant fits insideJoin's alignment padding, so the enum is exactly the same width asJoin.DDL plan construction takes one extra
Box::new(...)allocation per DDL statement — negligible because DDL plans are one-shot and not on the per-query hot path.The diff is mechanical: construction sites add
Box::new(...), the few field-destructuring patterns are converted to alet CreateExternalTable { … } = ce.as_ref();shape, and the FFI/proto crates need one*cmd/Box::new(cmd)adjustment at the type boundaries.A new
test_size_of_logical_planunit test pinssize_of::<LogicalPlan>() == 176and assertsDdlStatementstays smaller thanJoin, so future variant growth that would re-balloon the enum trips the test rather than silently regressing the planning hot path. (Same shape as the existingtest_size_of_exprinexpr.rs.)Are these changes tested?
create_external_table/create_function/ddlpass.cargo clippy --workspace --all-targets -- -D warningsclean.Are there any user-facing changes?
Yes (semantic API only). Code that pattern-matches
DdlStatement::CreateExternalTable(CreateExternalTable { … })must change to bind the box and deref. Code that constructs these variants must wrap withBox::new(...). No behavioral change.