You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2024/06/11 19:40:14 UTC
[PR] fix: Re-implement int decode methods using safe code [datafusion-comet]
andygrove opened a new pull request, #558:
URL: https://github.com/apache/datafusion-comet/pull/558
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2166839176
Thanks for the review @viirya
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#discussion_r1636576432
##########
core/src/parquet/read/values.rs:
##########
@@ -182,25 +182,6 @@ make_plain_dict_impl! { Int8Type, UInt8Type, Int16Type, UInt16Type, Int32Type, U
make_plain_dict_impl! { Int32DateType, Int64Type, FloatType, FLBAType }
make_plain_dict_impl! { DoubleType, Int64TimestampMillisType, Int64TimestampMicrosType }
-impl PlainDecoding for Int32To64Type {
Review Comment:
The updated macro is based on this impl and has replaced this impl
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2161927678
> That makes more sense to me. It's hard to measure in picoseconds if the system clock only provides nanosecond granularity.
That time was based on running ~75 million iterations and taking the average time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2161760028
> Is that really 839 `nano`seconds versus 68 `pico`seconds or am I interpreting the units incorrectly?
Yes, it really is. Here is the criterion code for displaying the time units.
```rust
pub fn time(ns: f64) -> String {
if ns < 1.0 {
format!("{:>6} ps", short(ns * 1e3))
} else if ns < 10f64.powi(3) {
format!("{:>6} ns", short(ns))
} else if ns < 10f64.powi(6) {
format!("{:>6} µs", short(ns / 1e3))
} else if ns < 10f64.powi(9) {
format!("{:>6} ms", short(ns / 1e6))
} else {
format!("{:>6} s", short(ns / 1e9))
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2166819246
> The newer Rust version requires alignments on `copy_nonoverlapping` call?
Just to be clear, this was always the requirement, but Rust 1.78 added debug assertions to catch violations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2161882504
Once tests pass, I'll see if I can turn this back into macros to avoid so much boilerplate
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#discussion_r1636577572
##########
core/src/parquet/read/values.rs:
##########
@@ -438,41 +419,92 @@ impl PlainDictDecoding for BoolType {
}
}
-// Shared implementation for int variants such as Int8 and Int16
-macro_rules! make_int_variant_impl {
- ($ty: ident, $native_ty: ty, $type_size: expr) => {
- impl PlainDecoding for $ty {
+macro_rules! impl_plain_decoding_int {
+ ($dst_type:ty, $copy_fn:ident, $type_width:expr) => {
+ impl PlainDecoding for $dst_type {
fn decode(src: &mut PlainDecoderInner, dst: &mut ParquetMutableVector, num: usize) {
- let src_data = &src.data;
let dst_slice = dst.value_buffer.as_slice_mut();
- let mut dst_offset = dst.num_values * $type_size;
- for _ in 0..num {
- unsafe {
- copy_nonoverlapping(
- &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; // Parquet stores Int8/Int16 using 4 bytes
- dst_offset += $type_size;
- }
+ let dst_offset = dst.num_values * $type_width;
+ $copy_fn(&src.data[src.offset..], &mut dst_slice[dst_offset..], num);
Review Comment:
note that we are calling a macro-generated function here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods using safe code [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2161661521
How would one even measure picoseconds. I expect that is a pseudo µ ? So `micro` not `pico` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2166812415
> Parquet decoding when converting between different integral types was using `copy_nonoverlapping` without meeting the precondition that both pointers were properly aligned.
The newer Rust version requires alignments on `copy_nonoverlapping` call?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2166808756
> I am questioning the earlier results now. Latest benchmark comparing safe version of `decode_i32_to_u16` vs unsafe version of `decode_i32_to_i16`. Now seeing 67 ns not 68 ps. Curious.
So unsafe version is slower?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods using safe code [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2161657329
Great benchmark Andy!
Is that really 839 `nano`seconds versus 68 `pico`seconds or am I interpreting the units incorrectly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2162959167
@parthchandra This is ready for review now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#discussion_r1637574415
##########
core/src/parquet/read/values.rs:
##########
@@ -438,41 +419,92 @@ impl PlainDictDecoding for BoolType {
}
}
-// Shared implementation for int variants such as Int8 and Int16
-macro_rules! make_int_variant_impl {
- ($ty: ident, $native_ty: ty, $type_size: expr) => {
- impl PlainDecoding for $ty {
+macro_rules! impl_plain_decoding_int {
+ ($dst_type:ty, $copy_fn:ident, $type_width:expr) => {
+ impl PlainDecoding for $dst_type {
fn decode(src: &mut PlainDecoderInner, dst: &mut ParquetMutableVector, num: usize) {
- let src_data = &src.data;
let dst_slice = dst.value_buffer.as_slice_mut();
- let mut dst_offset = dst.num_values * $type_size;
- for _ in 0..num {
- unsafe {
- copy_nonoverlapping(
- &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; // Parquet stores Int8/Int16 using 4 bytes
- dst_offset += $type_size;
- }
+ let dst_offset = dst.num_values * $type_width;
+ $copy_fn(&src.data[src.offset..], &mut dst_slice[dst_offset..], num);
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
}
fn skip(src: &mut PlainDecoderInner, num: usize) {
- let num_bytes = 4 * num; // Parquet stores Int8/Int16 using 4 bytes
- src.offset += num_bytes;
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
+ }
+ }
+ };
+}
+
+impl_plain_decoding_int!(Int8Type, copy_i32_to_i8, 1);
+impl_plain_decoding_int!(Int16Type, copy_i32_to_i16, 2);
+impl_plain_decoding_int!(Int32To64Type, copy_i32_to_i64, 4);
+
+// unsigned type require double the width and zeroes are written for the second half
+// perhaps because they are implemented as the next size up signed type?
+impl_plain_decoding_int!(UInt8Type, copy_i32_to_u8, 2);
Review Comment:
Hmm, why? I thought Int8 and UInt8 both are 8-bit width, no?
https://doc.rust-lang.org/book/ch03-02-data-types.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2161866998
> Now seeing 67 ns not 68 ps. Curious
That makes more sense to me. It's hard to measure in picoseconds if the system clock only provides nanosecond granularity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#discussion_r1638249606
##########
core/src/parquet/read/values.rs:
##########
@@ -438,41 +419,92 @@ impl PlainDictDecoding for BoolType {
}
}
-// Shared implementation for int variants such as Int8 and Int16
-macro_rules! make_int_variant_impl {
- ($ty: ident, $native_ty: ty, $type_size: expr) => {
- impl PlainDecoding for $ty {
+macro_rules! impl_plain_decoding_int {
+ ($dst_type:ty, $copy_fn:ident, $type_width:expr) => {
+ impl PlainDecoding for $dst_type {
fn decode(src: &mut PlainDecoderInner, dst: &mut ParquetMutableVector, num: usize) {
- let src_data = &src.data;
let dst_slice = dst.value_buffer.as_slice_mut();
- let mut dst_offset = dst.num_values * $type_size;
- for _ in 0..num {
- unsafe {
- copy_nonoverlapping(
- &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; // Parquet stores Int8/Int16 using 4 bytes
- dst_offset += $type_size;
- }
+ let dst_offset = dst.num_values * $type_width;
+ $copy_fn(&src.data[src.offset..], &mut dst_slice[dst_offset..], num);
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
}
fn skip(src: &mut PlainDecoderInner, num: usize) {
- let num_bytes = 4 * num; // Parquet stores Int8/Int16 using 4 bytes
- src.offset += num_bytes;
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
+ }
+ }
+ };
+}
+
+impl_plain_decoding_int!(Int8Type, copy_i32_to_i8, 1);
+impl_plain_decoding_int!(Int16Type, copy_i32_to_i16, 2);
+impl_plain_decoding_int!(Int32To64Type, copy_i32_to_i64, 4);
+
+// unsigned type require double the width and zeroes are written for the second half
+// perhaps because they are implemented as the next size up signed type?
+impl_plain_decoding_int!(UInt8Type, copy_i32_to_u8, 2);
Review Comment:
This confused me as well, but I am using the same type widths as the original code:
```
make_int_variant_impl!(Int8Type, i8, 1);
make_int_variant_impl!(UInt8Type, u8, 2);
make_int_variant_impl!(Int16Type, i16, 2);
make_int_variant_impl!(UInt16Type, u16, 4);
make_int_variant_impl!(UInt32Type, u32, 8);
```
This seems to be specific to the way Parquet represents unsigned types. I think Parquet only supports signed types so u8 becomes i16, u16 becomes i32, and so on
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#discussion_r1638260394
##########
core/src/parquet/read/values.rs:
##########
@@ -438,41 +419,92 @@ impl PlainDictDecoding for BoolType {
}
}
-// Shared implementation for int variants such as Int8 and Int16
-macro_rules! make_int_variant_impl {
- ($ty: ident, $native_ty: ty, $type_size: expr) => {
- impl PlainDecoding for $ty {
+macro_rules! impl_plain_decoding_int {
+ ($dst_type:ty, $copy_fn:ident, $type_width:expr) => {
+ impl PlainDecoding for $dst_type {
fn decode(src: &mut PlainDecoderInner, dst: &mut ParquetMutableVector, num: usize) {
- let src_data = &src.data;
let dst_slice = dst.value_buffer.as_slice_mut();
- let mut dst_offset = dst.num_values * $type_size;
- for _ in 0..num {
- unsafe {
- copy_nonoverlapping(
- &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; // Parquet stores Int8/Int16 using 4 bytes
- dst_offset += $type_size;
- }
+ let dst_offset = dst.num_values * $type_width;
+ $copy_fn(&src.data[src.offset..], &mut dst_slice[dst_offset..], num);
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
}
fn skip(src: &mut PlainDecoderInner, num: usize) {
- let num_bytes = 4 * num; // Parquet stores Int8/Int16 using 4 bytes
- src.offset += num_bytes;
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
+ }
+ }
+ };
+}
+
+impl_plain_decoding_int!(Int8Type, copy_i32_to_i8, 1);
+impl_plain_decoding_int!(Int16Type, copy_i32_to_i16, 2);
+impl_plain_decoding_int!(Int32To64Type, copy_i32_to_i64, 4);
+
+// unsigned type require double the width and zeroes are written for the second half
+// perhaps because they are implemented as the next size up signed type?
+impl_plain_decoding_int!(UInt8Type, copy_i32_to_u8, 2);
+impl_plain_decoding_int!(UInt16Type, copy_i32_to_u16, 4);
+impl_plain_decoding_int!(UInt32Type, copy_i32_to_u32, 8);
+
+macro_rules! generate_cast_to_unsigned {
+ ($name: ident, $src_type:ty, $dst_type:ty, $zero_value:expr) => {
+ pub fn $name(src: &[u8], dst: &mut [u8], num: usize) {
+ debug_assert!(
+ src.len() >= num * std::mem::size_of::<$src_type>(),
+ "Source slice is too small"
+ );
+ debug_assert!(
+ dst.len() >= num * std::mem::size_of::<$dst_type>() * 2,
+ "Destination slice is too small"
+ );
+
+ let src_ptr = src.as_ptr() as *const $src_type;
+ let dst_ptr = dst.as_mut_ptr() as *mut $dst_type;
+ unsafe {
+ for i in 0..num {
+ dst_ptr
+ .add(2 * i)
+ .write_unaligned(src_ptr.add(i).read_unaligned() as $dst_type);
+ // write zeroes
+ dst_ptr.add(2 * i + 1).write_unaligned($zero_value);
+ }
}
}
};
}
-make_int_variant_impl!(Int8Type, i8, 1);
-make_int_variant_impl!(UInt8Type, u8, 2);
-make_int_variant_impl!(Int16Type, i16, 2);
-make_int_variant_impl!(UInt16Type, u16, 4);
-make_int_variant_impl!(UInt32Type, u32, 8);
+generate_cast_to_unsigned!(copy_i32_to_u8, i32, u8, 0_u8);
+generate_cast_to_unsigned!(copy_i32_to_u16, i32, u16, 0_u16);
+generate_cast_to_unsigned!(copy_i32_to_u32, i32, u32, 0_u32);
+
+macro_rules! generate_cast_to_signed {
+ ($name: ident, $src_type:ty, $dst_type:ty, $zero_value:expr) => {
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#discussion_r1638365132
##########
core/src/parquet/read/values.rs:
##########
@@ -438,41 +419,92 @@ impl PlainDictDecoding for BoolType {
}
}
-// Shared implementation for int variants such as Int8 and Int16
-macro_rules! make_int_variant_impl {
- ($ty: ident, $native_ty: ty, $type_size: expr) => {
- impl PlainDecoding for $ty {
+macro_rules! impl_plain_decoding_int {
+ ($dst_type:ty, $copy_fn:ident, $type_width:expr) => {
+ impl PlainDecoding for $dst_type {
fn decode(src: &mut PlainDecoderInner, dst: &mut ParquetMutableVector, num: usize) {
- let src_data = &src.data;
let dst_slice = dst.value_buffer.as_slice_mut();
- let mut dst_offset = dst.num_values * $type_size;
- for _ in 0..num {
- unsafe {
- copy_nonoverlapping(
- &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; // Parquet stores Int8/Int16 using 4 bytes
- dst_offset += $type_size;
- }
+ let dst_offset = dst.num_values * $type_width;
+ $copy_fn(&src.data[src.offset..], &mut dst_slice[dst_offset..], num);
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
}
fn skip(src: &mut PlainDecoderInner, num: usize) {
- let num_bytes = 4 * num; // Parquet stores Int8/Int16 using 4 bytes
- src.offset += num_bytes;
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
+ }
+ }
+ };
+}
+
+impl_plain_decoding_int!(Int8Type, copy_i32_to_i8, 1);
+impl_plain_decoding_int!(Int16Type, copy_i32_to_i16, 2);
+impl_plain_decoding_int!(Int32To64Type, copy_i32_to_i64, 4);
+
+// unsigned type require double the width and zeroes are written for the second half
+// perhaps because they are implemented as the next size up signed type?
+impl_plain_decoding_int!(UInt8Type, copy_i32_to_u8, 2);
Review Comment:
Ah, I see. I forgot it. As Java doesn't support unsigned integer types, we need to use wider type to support it. For example, UInt8Array is read as Short, UInt32Array is read as Long, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#discussion_r1638962935
##########
core/src/parquet/read/values.rs:
##########
@@ -438,41 +419,92 @@ impl PlainDictDecoding for BoolType {
}
}
-// Shared implementation for int variants such as Int8 and Int16
macro_rules! make_int_variant_impl {
- ($ty: ident, $native_ty: ty, $type_size: expr) => {
- impl PlainDecoding for $ty {
+ ($dst_type:ty, $copy_fn:ident, $type_width:expr) => {
+ impl PlainDecoding for $dst_type {
fn decode(src: &mut PlainDecoderInner, dst: &mut ParquetMutableVector, num: usize) {
- let src_data = &src.data;
let dst_slice = dst.value_buffer.as_slice_mut();
- let mut dst_offset = dst.num_values * $type_size;
- for _ in 0..num {
- unsafe {
- copy_nonoverlapping(
- &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; // Parquet stores Int8/Int16 using 4 bytes
- dst_offset += $type_size;
- }
+ let dst_offset = dst.num_values * $type_width;
+ $copy_fn(&src.data[src.offset..], &mut dst_slice[dst_offset..], num);
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
}
fn skip(src: &mut PlainDecoderInner, num: usize) {
- let num_bytes = 4 * num; // Parquet stores Int8/Int16 using 4 bytes
- src.offset += num_bytes;
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
+ }
+ }
+ };
+}
+
+make_int_variant_impl!(Int8Type, copy_i32_to_i8, 1);
+make_int_variant_impl!(Int16Type, copy_i32_to_i16, 2);
+make_int_variant_impl!(Int32To64Type, copy_i32_to_i64, 4);
+
+// unsigned type require double the width and zeroes are written for the second half
+// perhaps because they are implemented as the next size up signed type?
+make_int_variant_impl!(UInt8Type, copy_i32_to_u8, 2);
+make_int_variant_impl!(UInt16Type, copy_i32_to_u16, 4);
+make_int_variant_impl!(UInt32Type, copy_i32_to_u32, 8);
+
+macro_rules! generate_cast_to_unsigned {
+ ($name: ident, $src_type:ty, $dst_type:ty, $zero_value:expr) => {
+ pub fn $name(src: &[u8], dst: &mut [u8], num: usize) {
+ debug_assert!(
+ src.len() >= num * std::mem::size_of::<$src_type>(),
+ "Source slice is too small"
+ );
+ debug_assert!(
+ dst.len() >= num * std::mem::size_of::<$dst_type>() * 2,
+ "Destination slice is too small"
+ );
+
+ let src_ptr = src.as_ptr() as *const $src_type;
+ let dst_ptr = dst.as_mut_ptr() as *mut $dst_type;
+ unsafe {
+ for i in 0..num {
+ dst_ptr
+ .add(2 * i)
+ .write_unaligned(src_ptr.add(i).read_unaligned() as $dst_type);
+ // write zeroes
+ dst_ptr.add(2 * i + 1).write_unaligned($zero_value);
+ }
}
Review Comment:
Is any possible we make source and destination aligned? So we can still use unsafe `copy_nonoverlapping`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#discussion_r1638964444
##########
core/src/parquet/read/values.rs:
##########
@@ -438,41 +419,92 @@ impl PlainDictDecoding for BoolType {
}
}
-// Shared implementation for int variants such as Int8 and Int16
macro_rules! make_int_variant_impl {
- ($ty: ident, $native_ty: ty, $type_size: expr) => {
- impl PlainDecoding for $ty {
+ ($dst_type:ty, $copy_fn:ident, $type_width:expr) => {
+ impl PlainDecoding for $dst_type {
fn decode(src: &mut PlainDecoderInner, dst: &mut ParquetMutableVector, num: usize) {
- let src_data = &src.data;
let dst_slice = dst.value_buffer.as_slice_mut();
- let mut dst_offset = dst.num_values * $type_size;
- for _ in 0..num {
- unsafe {
- copy_nonoverlapping(
- &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; // Parquet stores Int8/Int16 using 4 bytes
- dst_offset += $type_size;
- }
+ let dst_offset = dst.num_values * $type_width;
+ $copy_fn(&src.data[src.offset..], &mut dst_slice[dst_offset..], num);
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
}
fn skip(src: &mut PlainDecoderInner, num: usize) {
- let num_bytes = 4 * num; // Parquet stores Int8/Int16 using 4 bytes
- src.offset += num_bytes;
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
+ }
+ }
+ };
+}
+
+make_int_variant_impl!(Int8Type, copy_i32_to_i8, 1);
+make_int_variant_impl!(Int16Type, copy_i32_to_i16, 2);
+make_int_variant_impl!(Int32To64Type, copy_i32_to_i64, 4);
+
+// unsigned type require double the width and zeroes are written for the second half
+// perhaps because they are implemented as the next size up signed type?
+make_int_variant_impl!(UInt8Type, copy_i32_to_u8, 2);
+make_int_variant_impl!(UInt16Type, copy_i32_to_u16, 4);
+make_int_variant_impl!(UInt32Type, copy_i32_to_u32, 8);
+
+macro_rules! generate_cast_to_unsigned {
+ ($name: ident, $src_type:ty, $dst_type:ty, $zero_value:expr) => {
+ pub fn $name(src: &[u8], dst: &mut [u8], num: usize) {
+ debug_assert!(
+ src.len() >= num * std::mem::size_of::<$src_type>(),
+ "Source slice is too small"
+ );
+ debug_assert!(
+ dst.len() >= num * std::mem::size_of::<$dst_type>() * 2,
+ "Destination slice is too small"
+ );
+
+ let src_ptr = src.as_ptr() as *const $src_type;
+ let dst_ptr = dst.as_mut_ptr() as *mut $dst_type;
+ unsafe {
+ for i in 0..num {
+ dst_ptr
+ .add(2 * i)
+ .write_unaligned(src_ptr.add(i).read_unaligned() as $dst_type);
+ // write zeroes
+ dst_ptr.add(2 * i + 1).write_unaligned($zero_value);
+ }
}
Review Comment:
I think that would involve making an extra copy?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#discussion_r1637580192
##########
core/src/parquet/read/values.rs:
##########
@@ -438,41 +419,92 @@ impl PlainDictDecoding for BoolType {
}
}
-// Shared implementation for int variants such as Int8 and Int16
-macro_rules! make_int_variant_impl {
- ($ty: ident, $native_ty: ty, $type_size: expr) => {
- impl PlainDecoding for $ty {
+macro_rules! impl_plain_decoding_int {
+ ($dst_type:ty, $copy_fn:ident, $type_width:expr) => {
+ impl PlainDecoding for $dst_type {
fn decode(src: &mut PlainDecoderInner, dst: &mut ParquetMutableVector, num: usize) {
- let src_data = &src.data;
let dst_slice = dst.value_buffer.as_slice_mut();
- let mut dst_offset = dst.num_values * $type_size;
- for _ in 0..num {
- unsafe {
- copy_nonoverlapping(
- &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; // Parquet stores Int8/Int16 using 4 bytes
- dst_offset += $type_size;
- }
+ let dst_offset = dst.num_values * $type_width;
+ $copy_fn(&src.data[src.offset..], &mut dst_slice[dst_offset..], num);
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
}
fn skip(src: &mut PlainDecoderInner, num: usize) {
- let num_bytes = 4 * num; // Parquet stores Int8/Int16 using 4 bytes
- src.offset += num_bytes;
+ src.offset += 4 * num; // Parquet stores Int8/Int16 using 4 bytes
+ }
+ }
+ };
+}
+
+impl_plain_decoding_int!(Int8Type, copy_i32_to_i8, 1);
+impl_plain_decoding_int!(Int16Type, copy_i32_to_i16, 2);
+impl_plain_decoding_int!(Int32To64Type, copy_i32_to_i64, 4);
+
+// unsigned type require double the width and zeroes are written for the second half
+// perhaps because they are implemented as the next size up signed type?
+impl_plain_decoding_int!(UInt8Type, copy_i32_to_u8, 2);
+impl_plain_decoding_int!(UInt16Type, copy_i32_to_u16, 4);
+impl_plain_decoding_int!(UInt32Type, copy_i32_to_u32, 8);
+
+macro_rules! generate_cast_to_unsigned {
+ ($name: ident, $src_type:ty, $dst_type:ty, $zero_value:expr) => {
+ pub fn $name(src: &[u8], dst: &mut [u8], num: usize) {
+ debug_assert!(
+ src.len() >= num * std::mem::size_of::<$src_type>(),
+ "Source slice is too small"
+ );
+ debug_assert!(
+ dst.len() >= num * std::mem::size_of::<$dst_type>() * 2,
+ "Destination slice is too small"
+ );
+
+ let src_ptr = src.as_ptr() as *const $src_type;
+ let dst_ptr = dst.as_mut_ptr() as *mut $dst_type;
+ unsafe {
+ for i in 0..num {
+ dst_ptr
+ .add(2 * i)
+ .write_unaligned(src_ptr.add(i).read_unaligned() as $dst_type);
+ // write zeroes
+ dst_ptr.add(2 * i + 1).write_unaligned($zero_value);
+ }
}
}
};
}
-make_int_variant_impl!(Int8Type, i8, 1);
-make_int_variant_impl!(UInt8Type, u8, 2);
-make_int_variant_impl!(Int16Type, i16, 2);
-make_int_variant_impl!(UInt16Type, u16, 4);
-make_int_variant_impl!(UInt32Type, u32, 8);
+generate_cast_to_unsigned!(copy_i32_to_u8, i32, u8, 0_u8);
+generate_cast_to_unsigned!(copy_i32_to_u16, i32, u16, 0_u16);
+generate_cast_to_unsigned!(copy_i32_to_u32, i32, u32, 0_u32);
+
+macro_rules! generate_cast_to_signed {
+ ($name: ident, $src_type:ty, $dst_type:ty, $zero_value:expr) => {
Review Comment:
Seems `generate_cast_to_signed` doesn't need `zero_value`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods without `copy_nonoverlapping` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2161776154
> Is that really 839 `nano`seconds versus 68 `pico`seconds or am I interpreting the units incorrectly?
I am questioning the earlier results now. Latest benchmark comparing safe version of `decode_i32_to_u16` vs unsafe version of `decode_i32_to_i16`. Now seeing 67 ns not 68 ps. Curious.
```
parquet_decode/decode_i32_to_i16
time: [66.709 ns 67.213 ns 67.711 ns]
change: [-2.4520% -1.0484% +0.3895%] (p = 0.15 > 0.05)
No change in performance detected.
parquet_decode/decode_i32_to_u16
time: [589.31 ns 590.62 ns 592.21 ns]
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Re-implement some Parquet decode methods using safe code [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #558:
URL: https://github.com/apache/datafusion-comet/pull/558#issuecomment-2161643541
Comparison of safe code vs using unsafe read/write aligned:
```rust
pub fn copy_i32_to_i16(src: &[u8], dst: &mut [u8], num: usize) {
debug_assert!(src.len() >= num * 4, "Source slice is too small");
debug_assert!(dst.len() >= num * 2, "Destination slice is too small");
for i in 0..num {
let i32_value =
i32::from_le_bytes([src[i * 4], src[i * 4 + 1], src[i * 4 + 2], src[i * 4 + 3]]);
// Downcast to i16, potentially losing data
let i16_value = i32_value as i16;
let i16_bytes = i16_value.to_le_bytes();
dst[i * 2] = i16_bytes[0];
dst[i * 2 + 1] = i16_bytes[1];
}
}
pub fn copy_i32_to_i16_unsafe(src: &[u8], dst: &mut [u8], num: usize) {
debug_assert!(src.len() >= num * 4, "Source slice is too small");
debug_assert!(dst.len() >= num * 2, "Destination slice is too small");
let src_ptr = src.as_ptr() as *const i32;
let dst_ptr = dst.as_mut_ptr() as *mut i16;
unsafe {
for i in 0..num {
dst_ptr
.add(i)
.write_unaligned(src_ptr.add(i).read_unaligned() as i16);
}
}
}
```
```
parquet_decode/decode_i32_to_i16_safe
time: [839.29 ns 839.57 ns 839.93 ns]
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe
parquet_decode/decode_i32_to_i16_unsafe
time: [68.269 ps 68.400 ps 68.593 ps]
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) low mild
2 (2.00%) high mild
3 (3.00%) high severe
```
I am going to reimplement with the unaligned approach and see if that has any safety issues
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org