From 039d7c600f463f8f54ca910481f358c79007523b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Horstmann?= Date: Mon, 2 Nov 2020 15:52:36 +0100 Subject: [PATCH 1/6] ARROW-10461 BitChunks::remainder_bits needs to take offset into account --- rust/arrow/src/util/bit_chunk_iterator.rs | 59 +++++++++++++++++------ 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/rust/arrow/src/util/bit_chunk_iterator.rs b/rust/arrow/src/util/bit_chunk_iterator.rs index df6caf9a9cd..c9226ae496e 100644 --- a/rust/arrow/src/util/bit_chunk_iterator.rs +++ b/rust/arrow/src/util/bit_chunk_iterator.rs @@ -72,22 +72,23 @@ impl<'a> BitChunks<'a> { if bit_len == 0 { 0 } else { - let byte_len = ceil(bit_len, 8); - - let mut bits = 0; - for i in 0..byte_len { - let byte = unsafe { - std::ptr::read( - self.raw_data - .add(self.chunk_len * std::mem::size_of::() + i), - ) - }; - bits |= (byte as u64) << (i * 8); - } + let bit_offset = self.offset; + // number of bytes to read + // might be one more than sizeof(u64) if the offset is in the middle of a byte + let byte_len = ceil(bit_len + bit_offset, 8); + // pointer to remainder bytes after all complete chunks + let base = unsafe { + self.raw_data + .add(self.chunk_len * std::mem::size_of::()) + }; - let offset = self.offset as u64; + let mut bits = unsafe { std::ptr::read(base) } as u64 >> bit_offset; + for i in 1..byte_len { + let byte = unsafe { std::ptr::read(base.add(i)) }; + bits |= (byte as u64) << (i * 8 - bit_offset); + } - (bits >> offset) & ((1 << bit_len) - 1) + bits & ((1 << bit_len) - 1) } } @@ -192,7 +193,6 @@ mod tests { let result = bitchunks.into_iter().collect::>(); - //assert_eq!(vec![0b00010000, 0b00100000, 0b01000000, 0b10000000, 0b00000000, 0b00000001, 0b00000010, 0b11110100], result); assert_eq!( vec![0b1111010000000010000000010000000010000000010000000010000000010000], result @@ -214,10 +214,37 @@ mod tests { let result = bitchunks.into_iter().collect::>(); - //assert_eq!(vec![0b00010000, 0b00100000, 0b01000000, 0b10000000, 0b00000000, 0b00000001, 0b00000010, 0b11110100], result); assert_eq!( vec![0b1111010000000010000000010000000010000000010000000010000000010000], result ); } + + #[test] + fn test_iter_unaligned_remainder_bits_across_bytes() { + let input: &[u8] = &[0b00000000, 0b11111111]; + let buffer: Buffer = Buffer::from(input); + + let bitchunks = buffer.bit_chunks(6, 7); + + assert_eq!(7, bitchunks.remainder_len()); + assert_eq!(0b1111100, bitchunks.remainder_bits()); + } + + #[test] + fn test_iter_unaligned_remainder_bits_large() { + let input: &[u8] = &[ + 0b11111111, 0b00000000, 0b11111111, 0b00000000, 0b11111111, 0b00000000, + 0b11111111, 0b00000000, 0b11111111, + ]; + let buffer: Buffer = Buffer::from(input); + + let bitchunks = buffer.bit_chunks(2, 63); + + assert_eq!(63, bitchunks.remainder_len()); + assert_eq!( + 0b01000000_00111111_11000000_00111111_11000000_00111111_11000000_00111111, + bitchunks.remainder_bits() + ); + } } From 69cee9ed23c1232d4c9966f74b198d3dbf703e71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Horstmann?= Date: Mon, 2 Nov 2020 16:15:33 +0100 Subject: [PATCH 2/6] ARROW-10461 Fix/woraround dependency problem --- rust/arrow-flight/Cargo.toml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rust/arrow-flight/Cargo.toml b/rust/arrow-flight/Cargo.toml index abc9b1e5e7a..52a867bbef5 100644 --- a/rust/arrow-flight/Cargo.toml +++ b/rust/arrow-flight/Cargo.toml @@ -27,7 +27,10 @@ license = "Apache-2.0" [dependencies] arrow = { path = "../arrow", version = "3.0.0-SNAPSHOT" } -tonic = "0.3" +tonic = "0.3.1" +# version 0.3.1 has undeclared dependency on tower-util 0.3.1 +# https://github.com/hyperium/tonic/issues/480 +tower-util = "0.3.1" bytes = "0.5" prost = "0.6" prost-derive = "0.6" From 2e9251b71b8536968a098c36cd2bce9e56600e8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Horstmann?= Date: Tue, 3 Nov 2020 11:20:05 +0100 Subject: [PATCH 3/6] ARROW-10461 Add comments --- rust/arrow/src/util/bit_chunk_iterator.rs | 34 ++++++++++++++--------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/rust/arrow/src/util/bit_chunk_iterator.rs b/rust/arrow/src/util/bit_chunk_iterator.rs index c9226ae496e..21f39921438 100644 --- a/rust/arrow/src/util/bit_chunk_iterator.rs +++ b/rust/arrow/src/util/bit_chunk_iterator.rs @@ -22,8 +22,11 @@ use std::fmt::Debug; pub struct BitChunks<'a> { buffer: &'a Buffer, raw_data: *const u8, - offset: usize, + /// offset inside a byte, guaranteed to be between 0 and 7 (inclusive) + bit_offset: usize, + /// number of complete u64 chunks chunk_len: usize, + /// number of remaining bits, guaranteed to be between 0 and 63 (inclusive) remainder_len: usize, } @@ -32,11 +35,11 @@ impl<'a> BitChunks<'a> { assert!(ceil(offset + len, 8) <= buffer.len() * 8); let byte_offset = offset / 8; - let offset = offset % 8; + let bit_offset = offset % 8; let raw_data = unsafe { buffer.raw_data().add(byte_offset) }; - let chunk_bits = 64; + let chunk_bits = 8 * std::mem::size_of::(); let chunk_len = len / chunk_bits; let remainder_len = len & (chunk_bits - 1); @@ -44,7 +47,7 @@ impl<'a> BitChunks<'a> { BitChunks::<'a> { buffer: &buffer, raw_data, - offset, + bit_offset, chunk_len, remainder_len, } @@ -55,24 +58,26 @@ impl<'a> BitChunks<'a> { pub struct BitChunkIterator<'a> { buffer: &'a Buffer, raw_data: *const u8, - offset: usize, + bit_offset: usize, chunk_len: usize, index: usize, } impl<'a> BitChunks<'a> { + /// Returns the number of remaining bits, guaranteed to be between 0 and 63 (inclusive) #[inline] pub const fn remainder_len(&self) -> usize { self.remainder_len } + /// Returns the bitmask of remaining bits #[inline] pub fn remainder_bits(&self) -> u64 { let bit_len = self.remainder_len; if bit_len == 0 { 0 } else { - let bit_offset = self.offset; + let bit_offset = self.bit_offset; // number of bytes to read // might be one more than sizeof(u64) if the offset is in the middle of a byte let byte_len = ceil(bit_len + bit_offset, 8); @@ -92,12 +97,13 @@ impl<'a> BitChunks<'a> { } } + /// Returns an iterator over chunks of 64 bits represented as an u64 #[inline] pub const fn iter(&self) -> BitChunkIterator<'a> { BitChunkIterator::<'a> { buffer: self.buffer, raw_data: self.raw_data, - offset: self.offset, + bit_offset: self.bit_offset, chunk_len: self.chunk_len, index: 0, } @@ -128,7 +134,7 @@ impl Iterator for BitChunkIterator<'_> { std::ptr::read_unaligned((self.raw_data as *const u64).add(self.index)) }; - let combined = if self.offset == 0 { + let combined = if self.bit_offset == 0 { current } else { // cast to *const u64 should be fine since we are using read_unaligned @@ -138,8 +144,8 @@ impl Iterator for BitChunkIterator<'_> { (self.raw_data as *const u64).add(self.index + 1), ) }; - current >> self.offset - | (next & ((1 << self.offset) - 1)) << (64 - self.offset) + current >> self.bit_offset + | (next & ((1 << self.bit_offset) - 1)) << (64 - self.bit_offset) }; self.index += 1; @@ -222,13 +228,15 @@ mod tests { #[test] fn test_iter_unaligned_remainder_bits_across_bytes() { - let input: &[u8] = &[0b00000000, 0b11111111]; + let input: &[u8] = &[0b00111111, 0b11111100]; let buffer: Buffer = Buffer::from(input); + // remainder contains bits from both bytes + // result should be the highest 2 bits from first byte followed by lowest 5 bits of second bytes let bitchunks = buffer.bit_chunks(6, 7); assert_eq!(7, bitchunks.remainder_len()); - assert_eq!(0b1111100, bitchunks.remainder_bits()); + assert_eq!(0b1110000, bitchunks.remainder_bits()); } #[test] @@ -243,7 +251,7 @@ mod tests { assert_eq!(63, bitchunks.remainder_len()); assert_eq!( - 0b01000000_00111111_11000000_00111111_11000000_00111111_11000000_00111111, + 0b1000000_00111111_11000000_00111111_11000000_00111111_11000000_00111111, bitchunks.remainder_bits() ); } From b3973addd145ae9c5d841af09510fbdd0860cfde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Horstmann?= Date: Fri, 6 Nov 2020 17:06:17 +0100 Subject: [PATCH 4/6] ARROW-10461 Undo changes in flight cargo.toml --- rust/arrow-flight/Cargo.toml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rust/arrow-flight/Cargo.toml b/rust/arrow-flight/Cargo.toml index 52a867bbef5..abc9b1e5e7a 100644 --- a/rust/arrow-flight/Cargo.toml +++ b/rust/arrow-flight/Cargo.toml @@ -27,10 +27,7 @@ license = "Apache-2.0" [dependencies] arrow = { path = "../arrow", version = "3.0.0-SNAPSHOT" } -tonic = "0.3.1" -# version 0.3.1 has undeclared dependency on tower-util 0.3.1 -# https://github.com/hyperium/tonic/issues/480 -tower-util = "0.3.1" +tonic = "0.3" bytes = "0.5" prost = "0.6" prost-derive = "0.6" From 2b7341b325a607d725c971cafcced3003a60f1d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Horstmann?= Date: Sat, 7 Nov 2020 16:08:56 +0100 Subject: [PATCH 5/6] ARROW-10461 Comments and fixes for big-endian machines (untested) --- rust/arrow/src/buffer.rs | 2 ++ rust/arrow/src/util/bit_chunk_iterator.rs | 21 +++++++++++---------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/rust/arrow/src/buffer.rs b/rust/arrow/src/buffer.rs index 3512ae34261..b4745168a9e 100644 --- a/rust/arrow/src/buffer.rs +++ b/rust/arrow/src/buffer.rs @@ -414,6 +414,7 @@ where let remainder_bytes = ceil(left_chunks.remainder_len(), 8); let rem = op(left_chunks.remainder_bits(), right_chunks.remainder_bits()); + // we are counting its starting from the least significant bit, to to_le_bytes should be correct let rem = &rem.to_le_bytes()[0..remainder_bytes]; result .write_all(rem) @@ -448,6 +449,7 @@ where let remainder_bytes = ceil(left_chunks.remainder_len(), 8); let rem = op(left_chunks.remainder_bits()); + // we are counting its starting from the least significant bit, to to_le_bytes should be correct let rem = &rem.to_le_bytes()[0..remainder_bytes]; result .write_all(rem) diff --git a/rust/arrow/src/util/bit_chunk_iterator.rs b/rust/arrow/src/util/bit_chunk_iterator.rs index 21f39921438..1ba819c792c 100644 --- a/rust/arrow/src/util/bit_chunk_iterator.rs +++ b/rust/arrow/src/util/bit_chunk_iterator.rs @@ -124,31 +124,32 @@ impl Iterator for BitChunkIterator<'_> { #[inline] fn next(&mut self) -> Option { - if self.index >= self.chunk_len { + let index = self.index; + if index >= self.chunk_len { return None; } + let raw_data = (self.raw_data as *const u64); + // cast to *const u64 should be fine since we are using read_unaligned + // bit-packed buffers are stored starting with the least-significant byte first + // so when reading as u64 on a big-endian machine, the bytes need to be swapped #[allow(clippy::cast_ptr_alignment)] - let current = unsafe { - std::ptr::read_unaligned((self.raw_data as *const u64).add(self.index)) - }; + let current = unsafe { std::ptr::read_unaligned(raw_data.add(index)).to_le() }; let combined = if self.bit_offset == 0 { current } else { // cast to *const u64 should be fine since we are using read_unaligned #[allow(clippy::cast_ptr_alignment)] - let next = unsafe { - std::ptr::read_unaligned( - (self.raw_data as *const u64).add(self.index + 1), - ) - }; + let next = + unsafe { std::ptr::read_unaligned(raw_data.add(index + 1)).to_le() }; + current >> self.bit_offset | (next & ((1 << self.bit_offset) - 1)) << (64 - self.bit_offset) }; - self.index += 1; + self.index = index + 1; Some(combined) } From f695166e12fab64903215815e501b0c935770268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Horstmann?= Date: Sat, 7 Nov 2020 16:30:57 +0100 Subject: [PATCH 6/6] ARROW-10461 Clippy fixes --- rust/arrow/src/util/bit_chunk_iterator.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/rust/arrow/src/util/bit_chunk_iterator.rs b/rust/arrow/src/util/bit_chunk_iterator.rs index 1ba819c792c..ec10c6f1d41 100644 --- a/rust/arrow/src/util/bit_chunk_iterator.rs +++ b/rust/arrow/src/util/bit_chunk_iterator.rs @@ -129,19 +129,17 @@ impl Iterator for BitChunkIterator<'_> { return None; } - let raw_data = (self.raw_data as *const u64); + // cast to *const u64 should be fine since we are using read_unaligned below + #[allow(clippy::cast_ptr_alignment)] + let raw_data = self.raw_data as *const u64; - // cast to *const u64 should be fine since we are using read_unaligned // bit-packed buffers are stored starting with the least-significant byte first // so when reading as u64 on a big-endian machine, the bytes need to be swapped - #[allow(clippy::cast_ptr_alignment)] let current = unsafe { std::ptr::read_unaligned(raw_data.add(index)).to_le() }; let combined = if self.bit_offset == 0 { current } else { - // cast to *const u64 should be fine since we are using read_unaligned - #[allow(clippy::cast_ptr_alignment)] let next = unsafe { std::ptr::read_unaligned(raw_data.add(index + 1)).to_le() };