-
Notifications
You must be signed in to change notification settings - Fork 850
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 ScalarBuffer abstraction (#1811) #1820
Conversation
/// All [`ArrowNativeType`] are valid for all possible backing byte representations, and as | ||
/// a result they are "trivially safely transmutable". | ||
#[derive(Debug)] | ||
pub struct ScalarBuffer<T: ArrowNativeType> { |
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.
Is this to be public to users? If this is only for used internally in Array implementations.
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.
We definitely could make it crate local, I think a ScalarMutableBuffer
would be of great interest to parquet, and so I was just thinking we'd provide public safe APIs for interacting with buffers. Don't feel strongly
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.
Looks promising. 👍
use crate::datatypes::ArrowNativeType; | ||
use std::ops::Deref; | ||
|
||
/// Provides a safe API for interpreting a [`Buffer`] as a slice of [`ArrowNativeType`] |
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.
Am I right in understanding that the key use of this API will be to check the validity of buffer
once and subsequently "just access" it rather than having to do the checks subsequently (or skip them entirely)?
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.
Precisely - see #1825 for how this works in practice
Which issue does this PR close?
Part of #1811
Rationale for this change
This would allow replacing RawPtrBox with a safe abstraction
What changes are included in this PR?
This adds a new owning
ScalarBuffer
abstraction that wraps aBuffer
in an owning abstractionAre there any user-facing changes?
No