Skip to content
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.

Sign up for GitHub

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

chore: Remove unsafe use of from_raw_parts in Parquet decoder #549

Merged
merged 7 commits into from
Jun 11, 2024
Merged

chore: Remove unsafe use of from_raw_parts in Parquet decoder #549

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
17b83de
refactor make_int_variant_impl to remove unsafe use of from_raw_parts
andygrove Jun 10, 2024
f4cc518
fix
andygrove Jun 10, 2024
c07b677
refactor
andygrove Jun 10, 2024
52aa474
smaller change
andygrove Jun 10, 2024
58047d5
remove commented out code
andygrove Jun 10, 2024
43c42d0
remove in_ptr
andygrove Jun 10, 2024
b1d733b
simplify
andygrove Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 4 additions & 32 deletions core/src/parquet/read/values.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,49 +443,21 @@ macro_rules! make_int_variant_impl {
($ty: ident, $native_ty: ty, $type_size: expr) => {
impl PlainDecoding for $ty {
fn decode(src: &mut PlainDecoderInner, dst: &mut ParquetMutableVector, num: usize) {
let num_bytes = 4 * num; // Parquet stores Int8/Int16 using 4 bytes

let src_data = &src.data;
let mut src_offset = src.offset;
let dst_slice = dst.value_buffer.as_slice_mut();
let mut dst_offset = dst.num_values * $type_size;

let mut i = 0;
let mut in_ptr = &src_data[src_offset..] as *const [u8] as *const u8 as *const u32;

while num - i >= 32 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this loop is effectively not needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I combined the two loops into one. I explained more in #549 (comment)

unsafe {
let in_slice = std::slice::from_raw_parts(in_ptr, 32);
Copy link
Member Author

@andygrove andygrove Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unsafe because there is no guarantee that the buffer is properly aligned for u32 type (in_ptr is defined earier as let mut in_ptr = &src_data[src_offset..] as *const [u8] as *const u8 as *const u32;).


for n in 0..32 {
copy_nonoverlapping(
in_slice[n..].as_ptr() as *const $native_ty,
&mut dst_slice[dst_offset] as *mut u8 as *mut $native_ty,
1,
);
i += 1;
dst_offset += $type_size;
}
in_ptr = in_ptr.offset(32);
}
}

src_offset += i * 4;

(0..(num - i)).for_each(|_| {
for _ in 0..num {
unsafe {
copy_nonoverlapping(
&src_data[src_offset..] as *const [u8] as *const u8
&src_data[src.offset..] as *const [u8] as *const u8
as *const $native_ty,
&mut dst_slice[dst_offset] as *mut u8 as *mut $native_ty,
1,
);
}
src_offset += 4;
src.offset += 4; // Parquet stores Int8/Int16 using 4 bytes
dst_offset += $type_size;
});

src.offset += num_bytes;
}
}

fn skip(src: &mut PlainDecoderInner, num: usize) {
Expand Down
Loading