Avoid expanding unreferenced struct plan fields - #9254
Draft
joseph-isaacs wants to merge 1 commit into
Draft
Conversation
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Merging this PR will improve performance by 83.01%
Performance Changes
Tip Curious why this is faster? Comment Comparing Footnotes
|
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.
Summary
GetItem(root)expressions intact when partitioning a struct planSelect(root)into only the selected fieldsRoot cause
ExpressionStructRuleeagerly built a bound pack containing every struct field before it knewwhich fields the query referenced. On the 105-field ClickBench schema, every rule application
constructed 105
GetItemexpressions. Binding each node resolved its return dtype, repeatedlydeserializing lazy field dtypes even for a one-column query.
The expression is already bound, and the bound field annotator can partition direct field access
without expanding it. This change only synthesizes field accesses where
Selector a bare rootsemantically requires them.
Performance
The benchmark opens all 100 ClickBench Vortex shards, filters
AdvEngineID != 0, projectsAdvEngineID, warms planning once, and reports the median of 25 complete planning passes.BoundExpressionstackThis is a 66.6% planning reduction from the bound baseline and a 70.3% reduction from the original
stack. Execution is unchanged within run-to-run noise.
A matched Samply planning-only profile over 500 all-file passes reduced main-thread CPU from
8.22 s to 1.23 s. The previous hot stack through
BoundExpression::try_new,GetItem::return_dtype, and lazy dtype deserialization is absent after the change.Stack
This draft is stacked on #9244 and contains one signed commit.
Validation
RUSTC_WRAPPER= cargo nextest run -p vortex-layout -p vortex-scan-v2— 215 passedRUSTC_WRAPPER= cargo clippy --all-targets --all-features -- -D warningsRUSTC_WRAPPER= cargo test --doc -p vortex-layout -p vortex-scan-v2cargo +nightly fmt --all -- --checkgit diff --check