parthchandra commented on code in PR #27115:
URL: https://github.com/apache/datafusion/pull/27115#discussion_r4528868297
##########
datafusion/functions-aggregate/src/min_max/min_max_struct.rs:
##########
@@ -625,4 +628,41 @@ mod tests {
assert_eq!(int_array.value(0), 4);
assert_eq!(str_array.value(0), "d");
}
+
+ /// Regression test for
<https://github.com/apache/datafusion/issues/26891>:
+ /// `EmitTo::First(n)` must release the memory accounted for the emitted
+ /// groups, not just their row counts.
+ #[test]
+ fn test_min_max_struct_size_after_emit_first() {
+ let long = "x".repeat(4096);
+ let strs: Vec<String> = (0..4).map(|i| format!("{i}{long}")).collect();
+ let array = create_test_struct_array(
+ vec![Some(1), Some(2), Some(3), Some(4)],
+ strs.iter().map(|s| Some(s.as_str())).collect(),
+ );
+ let data_type = array.data_type().clone();
+ let values = vec![Arc::new(array) as ArrayRef];
+
+ let mut accumulator =
MinMaxStructAccumulator::new_min(data_type.clone());
+ accumulator
+ .update_batch(&values, &[0, 1, 2, 3], None, 4)
Review Comment:
Good test. The assert_eq!(accumulator.size(), 0) after emitting everything
is the key check, it would fail before the fix.
##########
datafusion/functions-aggregate/src/min_max/min_max_struct.rs:
##########
@@ -306,7 +307,9 @@ impl MinMaxStructState {
let first_min_maxes = split_vec_min_alloc(&mut self.min_max,
n);
let first_data_capacity: usize = first_min_maxes
.iter()
- .map(|opt| opt.as_ref().map(|s| s.len()).unwrap_or(0))
+ .map(|opt| {
Review Comment:
Looks good. This matches how set_value adds memory (line 197 and 203 use
get_array_memory_size()), so now add and subtract use the same unit. The old
s.len() was always 1 per group, which is why memory kept growing. Nice fix.
--
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]
