tustvold commented on code in PR #2231:
URL: https://github.com/apache/arrow-rs/pull/2231#discussion_r933626962
##########
parquet/src/util/bit_util.rs:
##########
@@ -180,59 +179,32 @@ pub fn get_bit(data: &[u8], i: usize) -> bool {
/// bit packed or byte aligned fashion.
pub struct BitWriter {
buffer: Vec<u8>,
- max_bytes: usize,
buffered_values: u64,
- byte_offset: usize,
- bit_offset: usize,
- start: usize,
+ bit_offset: u8,
}
impl BitWriter {
pub fn new(max_bytes: usize) -> Self {
Self {
- buffer: vec![0; max_bytes],
- max_bytes,
+ buffer: Vec::with_capacity(max_bytes),
Review Comment:
An obvious benefit from this is we no longer zero-initialize this data,
which may improve performance (will run benchmarks tomorrow)
##########
parquet/src/util/bit_util.rs:
##########
@@ -138,7 +137,7 @@ where
/// This function should be removed after
/// [`int_roundings`](https://github.com/rust-lang/rust/issues/88581) is
stable.
#[inline]
-pub fn ceil(value: i64, divisor: i64) -> i64 {
+pub fn ceil<T: num::Integer>(value: T, divisor: T) -> T {
Review Comment:
Drive by cleanup to eliminate some unnecessary casting
##########
parquet/src/util/bit_util.rs:
##########
@@ -241,86 +213,62 @@ impl BitWriter {
#[inline]
pub fn flush_buffer(&mut self) -> &[u8] {
self.flush();
- &self.buffer()[0..self.byte_offset]
+ self.buffer()
}
/// Clears the internal state so the buffer can be reused.
#[inline]
pub fn clear(&mut self) {
+ self.buffer.clear();
self.buffered_values = 0;
- self.byte_offset = self.start;
Review Comment:
This could perhaps be considered a behaviour change, previously a `start`
parameter was passed in the constructor and resetting the writer would reset to
this offset. In practice this functionality was never being used, and I found
it very confusing, so I just removed it
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
