-
Notifications
You must be signed in to change notification settings - Fork 31
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 an ArrayBuffer that declares alignment #1720
base: develop
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,8 @@ use crate::validity::{ArrayValidity, LogicalValidity, Validity, ValidityMetadata | |
use crate::variants::{PrimitiveArrayTrait, VariantsVTable}; | ||
use crate::visitor::{ArrayVisitor, VisitorVTable}; | ||
use crate::{ | ||
impl_encoding, ArrayData, ArrayLen, ArrayTrait, Canonical, IntoArrayData, IntoCanonical, | ||
impl_encoding, ArrayBuffer, ArrayData, ArrayLen, ArrayTrait, Canonical, IntoArrayData, | ||
IntoCanonical, | ||
}; | ||
|
||
mod compute; | ||
|
@@ -60,7 +61,7 @@ impl PrimitiveArray { | |
.to_metadata(length) | ||
.vortex_expect("Invalid validity"), | ||
}), | ||
Some(buffer), | ||
Some(ArrayBuffer::new_with_alignment(buffer, ptype.byte_width())), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should constructor just accept ArrayBuffer directly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should accept a ScalarBuffer, and then the PType can be extracted. For now it's a bit annoying to take an ArrayBuffer and then make sure alignment matches when I'm about to change it anyway |
||
validity.into_array().into_iter().collect_vec().into(), | ||
StatsSet::default(), | ||
) | ||
|
@@ -101,7 +102,7 @@ impl PrimitiveArray { | |
}) | ||
} | ||
|
||
pub fn buffer(&self) -> &Buffer { | ||
pub fn buffer(&self) -> &ArrayBuffer { | ||
self.as_ref() | ||
.buffer() | ||
.vortex_expect("Missing buffer in PrimitiveArray") | ||
|
@@ -132,11 +133,14 @@ impl PrimitiveArray { | |
T::PTYPE, | ||
self.ptype(), | ||
); | ||
self.into_buffer().into_vec::<T>().unwrap_or_else(|b| { | ||
let (prefix, values, suffix) = unsafe { b.as_ref().align_to::<T>() }; | ||
assert!(prefix.is_empty() && suffix.is_empty()); | ||
Vec::from(values) | ||
}) | ||
self.into_buffer() | ||
.into_inner() | ||
.into_vec::<T>() | ||
.unwrap_or_else(|b| { | ||
let (prefix, values, suffix) = unsafe { b.as_ref().align_to::<T>() }; | ||
assert!(prefix.is_empty() && suffix.is_empty()); | ||
Vec::from(values) | ||
}) | ||
} | ||
|
||
pub fn get_as_cast<T: NativePType>(&self, idx: usize) -> T { | ||
|
@@ -156,10 +160,10 @@ impl PrimitiveArray { | |
"can't reinterpret cast between integers of two different widths" | ||
); | ||
|
||
PrimitiveArray::new(self.buffer().clone(), ptype, self.validity()) | ||
PrimitiveArray::new(self.buffer().clone().into_inner(), ptype, self.validity()) | ||
} | ||
|
||
pub fn into_buffer(self) -> Buffer { | ||
pub fn into_buffer(self) -> ArrayBuffer { | ||
self.into_array() | ||
.into_buffer() | ||
.vortex_expect("PrimitiveArray must have a buffer") | ||
|
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.
For now, I expect this will fail. I can fix this up in the PR to add ScalarBuffer by specifying a runtime alignment.