Rich-T-kid commented on code in PR #11423:
URL: https://github.com/apache/arrow-rs/pull/11423#discussion_r4213752957
##########
parquet/src/arrow/buffer/dictionary_buffer.rs:
##########
@@ -172,9 +249,14 @@ impl<K: ArrowNativeType + Ord, V: OffsetSizeTrait>
DictionaryBuffer<K, V> {
values
};
+ let keys_len = keys.len();
+ let keys_buffer = Buffer::from_vec(keys);
+ // Stash a clone for next-batch reclamation (succeeds once the
consumer drops its array).
Review Comment:
```suggestion
```
##########
parquet/src/arrow/buffer/dictionary_buffer.rs:
##########
@@ -38,16 +38,43 @@ use std::sync::Arc;
/// An array of variable length byte arrays that are potentially dictionary
encoded
/// and can be converted into a corresponding [`ArrayRef`]
pub enum DictionaryBuffer<K: ArrowNativeType, V: OffsetSizeTrait> {
- Dict { keys: Vec<K>, values: ArrayRef },
- Values { values: OffsetBuffer<V> },
+ Dict {
+ keys: Vec<K>,
+ values: ArrayRef,
+ /// Upper bound on key values (`(1u64 << bit_width) - 1`); if `<=
values.len()`, `into_array` skips bounds validation. `None` means validate.
+ key_upper_bound: Option<u64>,
Review Comment:
we can make this a bit cleaner
##########
parquet/src/arrow/record_reader/mod.rs:
##########
@@ -208,6 +208,14 @@ where
self.values.take().unwrap_or_else(|| V::with_capacity(0))
}
+ /// Install a pre-seeded values buffer for the next read, letting callers
+ /// recycle allocations from the previous batch. No-op if a buffer is
already present.
Review Comment:
```suggestion
/// Install a pre-seeded values buffer for the next read, letting callers
/// recycle allocations from the previous batch.
```
##########
parquet/src/arrow/array_reader/byte_array_dictionary.rs:
##########
@@ -239,14 +249,28 @@ where
let buffer = self.record_reader.consume_record_data();
let null_buffer = self.record_reader.consume_compact_bitmap();
+ // `into_array_with_keys_buffer_sink` stashes the newly-constructed
keys
+ // `Buffer` here (when taking the dictionary-preserving fast path).
+ // On the next `read_records` call we try to reclaim this allocation.
Review Comment:
```suggestion
// `into_array_with_keys_buffer_sink` stashes the newly-constructed
keys
// `Buffer`. On the next `read_records` call
// we try to reclaim this allocation.
```
##########
parquet/src/arrow/buffer/dictionary_buffer.rs:
##########
@@ -117,40 +171,63 @@ impl<K: ArrowNativeType + Ord, V: OffsetSizeTrait>
DictionaryBuffer<K, V> {
spilled.extend_from_dictionary(keys.as_slice(),
dict_offsets, dict_values)?;
}
- *self = Self::Values { values: spilled };
+ *self = Self::Values {
+ values: spilled,
+ recycled_keys: None,
+ };
match self {
- Self::Values { values } => Ok(values),
+ Self::Values { values, .. } => Ok(values),
Self::Dict { .. } => unreachable!(),
}
}
}
}
- /// Converts this into an [`ArrayRef`] with the provided `data_type` and
`null_buffer`
+ /// Converts this into an [`ArrayRef`] with the provided `data_type` and
`null_buffer`.
+ #[cfg_attr(not(test), expect(dead_code))]
Review Comment:
should remove this
--
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]
