comphead commented on code in PR #26154:
URL: https://github.com/apache/datafusion/pull/26154#discussion_r4265277039
##########
datafusion/functions-aggregate/src/min_max/min_max_bytes.rs:
##########
@@ -543,6 +557,103 @@ mod tests {
use super::*;
use arrow::array::StringArray;
+ #[test]
+ fn size_counts_outer_capacity() {
+ let mut state = MinMaxBytesState::new(DataType::Utf8);
+ assert_eq!(state.size(), 0);
+
+ state.min_max = Vec::with_capacity(16);
+ state.min_max.resize(3, None);
+ assert_eq!(
+ state.size(),
+ state.min_max.capacity() * size_of::<Option<Vec<u8>>>()
+ );
+ }
+
+ #[test]
+ fn size_retains_inner_capacity() -> Result<()> {
+ for is_min in [true, false] {
Review Comment:
The three new tests overlap.
- The accounting lives in `MinMaxBytesState` and `is_min` only changes the
comparison, so this loop runs the same accounting twice.
- `size_counts_outer_capacity` is covered if
`size_tracks_growth_and_emit_reuse` starts from `Vec::with_capacity(16)`, since
it already asserts `size()` after `resize`, partial emit and full emit.
One test would be enough: grow, replace 128 bytes with 1 byte (assert
`size()` stays at or above 128, which is the regression in the PR description),
partial emit, full emit, reuse. The repeated `capacity() *
size_of::<Option<Vec<u8>>>()` can become one helper that recomputes the
expected size by traversal. That also removes the manual `- emitted_capacity`
arithmetic.
##########
datafusion/functions-aggregate/src/min_max/min_max_bytes.rs:
##########
@@ -528,13 +535,20 @@ impl MinMaxBytesState {
.map(|opt| opt.as_ref().map(|s| s.len()).unwrap_or(0))
.sum();
self.total_data_bytes -= first_data_capacity;
+ self.total_data_capacity -= first_min_maxes
+ .iter()
+ .flatten()
+ .map(Vec::capacity)
+ .sum::<usize>();
(first_data_capacity, first_min_maxes)
Review Comment:
`first_min_maxes` is now traversed twice, once for the lengths and once for
the capacities. Both can be taken in one pass, and the `map(...unwrap_or(0))`
goes away:
```rust
let (bytes, capacity) = first_min_maxes
.iter()
.flatten()
.fold((0, 0), |(b, c), v| (b + v.len(), c + v.capacity()));
self.total_data_bytes -= bytes;
self.total_data_capacity -= capacity;
(bytes, first_min_maxes)
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
