Reduce size of Expr struct
#16207
Merged
Reduce size of Expr struct
#16207
Conversation
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
github-actions
bot
added
sql
Dandandan
reviewed
May 29, 2025
| @@ -330,7 +331,7 @@ pub enum Expr { | |||
| /// [`ExprFunctionExt`]: crate::expr_fn::ExprFunctionExt | |||
| AggregateFunction(AggregateFunction), | |||
| /// Call a window function with a set of arguments. | |||
| WindowFunction(WindowFunction), | |||
| WindowFunction(Box<WindowFunction>), | |||
There was a problem hiding this comment.
I wonder, what is now 144 bytes?
There was a problem hiding this comment.
Good question! I'm fairly new to Rust; what's a good way to analyze the enum variant sizes?
There was a problem hiding this comment.
It looks like Alias might be the culprit here (with some padding):
size_of::<Alias>() is 136, followed by size_of::<(DataType, Column)>() (alias OuterReferenceColumn) with 128.
There was a problem hiding this comment.
Maybe as a follow on experiment we can try to box Alias and OuterReferenceColumn too -- but I recommend not in this PR
There was a problem hiding this comment.
Agree - although it seems the size of the changes will be lower than 272 to 144 (I hoped maybe there would be a single one to reduce it from 144 -> 80).
Thanks for checking!
There was a problem hiding this comment.
Turns out I was doing something else and got the size down to 128:
Hopefully this will merge in a bit
|
🤖 |
|
I kicked off some benchmarks for this PR -- thank you @hendrikmakait 🙏 |
alamb
reviewed
May 30, 2025
There was a problem hiding this comment.
Thank you so much @hendrikmakait and @Dandandan
Assuming the benchmarks look good I think we should merge this PR.
Here is hoping for some improved performance: 🙏
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
alamb
approved these changes
May 30, 2025
There was a problem hiding this comment.
The planning benchmarks look really good to me. Thanks again @hendrikmakait
|
I'll leave this open for a few days to gather any remaining feedback |
|
I plan to merge this tomorrow so it can be included in DataFusion 48.0.0 |
|
🚀 -- thanks again @hendrikmakait |
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?
Exprstruct #16199.What changes are included in this PR?
ExprExpr::WindowFunction(WindowFunction)-->Expr::WindowFunction(Box<WindowFunction>)-- which drops the size ofExprfrom 272 to 144 bytesAre these changes tested?
Are there any user-facing changes?
No