-
Notifications
You must be signed in to change notification settings - Fork 847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Row size methods (#3160) #3163
Conversation
arrow/src/row/interner.rs
Outdated
+ self.values.buffer_size() | ||
+ self.values.buffer_size() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
twice?
/// Includes the size of `Self`. | ||
pub fn size(&self) -> usize { | ||
self.data_type.size() + std::mem::size_of::<Self>() | ||
- std::mem::size_of::<DataType>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why minus size of DataType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to count for options
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Options is counted by size_of<Self>
we need to subtract DataType as otherwise it is counted twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.data_type.size()
isn't equal to std::mem::size_of::<DataType>()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is that, plus any additional memory from nested fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, - std::mem::size_of::<DataType>()
is for data_type: DataType
which is already counted in by std::mem::size_of::<Self>()
?
Benchmark runs are scheduled for baseline = fa51333 and contender = 12a67b9. 12a67b9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3160
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?