Skip to content

Commit a8b5fb3

Browse files
viiryaIsaac
andcommitted
fix: release adapter row indices and first/last scoreboard in shrink_to_fit
GroupsAccumulatorAdapter keeps the capacity of each group's row indices after a batch, and FirstLastGroupsAccumulator truncates its per-batch scoreboard bitmap without releasing its capacity. Release both in shrink_to_fit, and cover them with skewed ANY_VALUE and ordered FIRST_VALUE tests. The hash stream test now reuses collect_under_limit and checks early_emit_count. Co-authored-by: Isaac <[email protected]>
1 parent 32b641f commit a8b5fb3

5 files changed

Lines changed: 96 additions & 22 deletions

File tree

‎datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -660,6 +660,15 @@ impl GroupsAccumulator for GroupsAccumulatorAdapter {
660660

661661
fn shrink_to_fit(&mut self) {
662662
self.free_scratch();
663+
// Each group keeps the capacity of its row indices after a batch, so
664+
// a group that had many rows in the last batch still holds them
665+
let mut freed = 0;
666+
for state in &mut self.states {
667+
if state.indices.is_empty() {
668+
freed += std::mem::take(&mut state.indices).allocated_size();
669+
}
670+
}
671+
self.free_allocation(freed);
663672
}
664673

665674
fn convert_to_state(

‎datafusion/functions-aggregate/src/first_last.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -882,6 +882,18 @@ impl<S: ValueState + 'static> GroupsAccumulator for FirstLastGroupsAccumulator<S
882882
+ self.extreme_of_each_group_buf.0.capacity() * size_of::<usize>()
883883
+ self.extreme_of_each_group_buf.1.capacity() / 8
884884
}
885+
886+
fn shrink_to_fit(&mut self) {
887+
// `take_state` truncates the per-batch scoreboard, which keeps its
888+
// capacity for every group seen before the emit. Every user of the
889+
// scoreboard accepts it all-false, so rebuild it at the current size.
890+
let num_groups = self.extreme_of_each_group_buf.1.len();
891+
let mut scoreboard = BooleanBufferBuilder::new(num_groups);
892+
scoreboard.append_n(num_groups, false);
893+
self.extreme_of_each_group_buf.1 = scoreboard;
894+
self.extreme_buf_dirty = false;
895+
}
896+
885897
fn convert_to_state(
886898
&self,
887899
values: &[ArrayRef],

‎datafusion/physical-plan/src/aggregates/clustered_partial_stream.rs‎

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -424,9 +424,9 @@ mod tests {
424424
use crate::aggregates::GroupClusteringMode;
425425
use crate::aggregates::PhysicalGroupBy;
426426
use crate::aggregates::clustered_test_util::{
427-
aggregate_over_sorted_input, assert_completed_groups_emitted_under_limit,
428-
assert_output_unchanged_under_limit, collect_under_limit, distinct_key_batches,
429-
full_batches, sum_over_sorted_input,
427+
aggregate_expr_over_sorted_input, aggregate_over_sorted_input,
428+
assert_completed_groups_emitted_under_limit, assert_output_unchanged_under_limit,
429+
collect_under_limit, distinct_key_batches, full_batches, sum_over_sorted_input,
430430
};
431431
use crate::test::TestMemoryExec;
432432
use arrow::array::{ArrayRef, Float64Array, Int64Array, ListArray};
@@ -438,6 +438,7 @@ mod tests {
438438
use datafusion_execution::runtime_env::RuntimeEnvBuilder;
439439
use datafusion_functions_aggregate::any_value::any_value_udaf;
440440
use datafusion_functions_aggregate::approx_distinct::approx_distinct_udaf;
441+
use datafusion_functions_aggregate::first_last::first_value_udaf;
441442
use datafusion_functions_aggregate::sum::sum_udaf;
442443
use datafusion_physical_expr::PhysicalSortExpr;
443444
use datafusion_physical_expr::aggregate::AggregateExprBuilder;
@@ -586,6 +587,45 @@ mod tests {
586587
Ok(())
587588
}
588589

590+
/// Like [`completed_groups_release_accumulator_scratch`], but the active
591+
/// group holds most rows of the batch. The `GroupsAccumulatorAdapter` of
592+
/// `ANY_VALUE` keeps the row indices it collected for each group (32 KB for
593+
/// the 8191 rows of the active group), so it must release them too.
594+
#[tokio::test]
595+
async fn active_group_releases_adapter_indices() -> Result<()> {
596+
let keys = std::iter::once(0)
597+
.chain(std::iter::repeat_n(1, 8191))
598+
.collect();
599+
let plan = aggregate_over_sorted_input(
600+
AggregateMode::Partial,
601+
&["k"],
602+
vec![vec![keys]],
603+
any_value_udaf(),
604+
)?;
605+
assert_eq!(plan.group_clustering_mode(), &GroupClusteringMode::Full);
606+
assert_output_unchanged_under_limit(&plan, 16 * 1024).await
607+
}
608+
609+
/// Like [`completed_groups_release_accumulator_scratch`], with the native
610+
/// `FIRST_VALUE(v ORDER BY v)` accumulator. It keeps a bitmap of the
611+
/// groups each batch updated, which grows to one bit per group (16 KB for
612+
/// 131072 groups), so it must release that bitmap too.
613+
#[tokio::test]
614+
async fn completed_groups_release_first_value_bitmap() -> Result<()> {
615+
let plan = aggregate_expr_over_sorted_input(
616+
AggregateMode::Partial,
617+
&["k"],
618+
vec![vec![(0..131072).collect()]],
619+
first_value_udaf(),
620+
|builder, schema| {
621+
Ok(builder
622+
.order_by(vec![PhysicalSortExpr::new_default(col("v", schema)?)]))
623+
},
624+
)?;
625+
assert_eq!(plan.group_clustering_mode(), &GroupClusteringMode::Full);
626+
assert_output_unchanged_under_limit(&plan, 16 * 1024).await
627+
}
628+
589629
/// With partial group clustering, the stream emits every state under
590630
/// memory pressure, so the output is not compared. After the emit, the
591631
/// accumulators must release their per-batch scratch, or reserving the

‎datafusion/physical-plan/src/aggregates/clustered_test_util.rs‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
use std::sync::Arc;
88

99
use arrow::array::{ArrayRef, Int64Array};
10-
use arrow::datatypes::{DataType, Field, Schema};
10+
use arrow::datatypes::{DataType, Field, Schema, SchemaRef};
1111
use arrow::record_batch::RecordBatch;
1212
use datafusion_common::test_util::batches_to_string;
1313
use datafusion_common::{DataFusionError, Result};
@@ -75,6 +75,19 @@ pub(super) fn aggregate_over_sorted_input(
7575
keys: &[&str],
7676
batches: Vec<Vec<Vec<i64>>>,
7777
udaf: Arc<AggregateUDF>,
78+
) -> Result<AggregateExec> {
79+
aggregate_expr_over_sorted_input(mode, keys, batches, udaf, |builder, _| Ok(builder))
80+
}
81+
82+
/// Like [`aggregate_over_sorted_input`], but `configure` can change the
83+
/// aggregate expression, given the input schema. For example, it can add
84+
/// `ORDER BY v`.
85+
pub(super) fn aggregate_expr_over_sorted_input(
86+
mode: AggregateMode,
87+
keys: &[&str],
88+
batches: Vec<Vec<Vec<i64>>>,
89+
udaf: Arc<AggregateUDF>,
90+
configure: impl FnOnce(AggregateExprBuilder, &SchemaRef) -> Result<AggregateExprBuilder>,
7891
) -> Result<AggregateExec> {
7992
let fields = keys
8093
.iter()
@@ -88,10 +101,13 @@ pub(super) fn aggregate_over_sorted_input(
88101
.collect::<Result<_>>()?,
89102
);
90103
let aggregate_exprs = vec![Arc::new(
91-
AggregateExprBuilder::new(Arc::clone(&udaf), vec![col("v", &schema)?])
92-
.schema(Arc::clone(&schema))
93-
.alias(udaf.name())
94-
.build()?,
104+
configure(
105+
AggregateExprBuilder::new(Arc::clone(&udaf), vec![col("v", &schema)?]),
106+
&schema,
107+
)?
108+
.schema(Arc::clone(&schema))
109+
.alias(udaf.name())
110+
.build()?,
95111
)];
96112

97113
let (input_schema, group_by) = if mode == AggregateMode::Final {

‎datafusion/physical-plan/src/aggregates/hash_stream.rs‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -843,6 +843,7 @@ mod tests {
843843

844844
use super::*;
845845
use crate::aggregates::GroupClusteringMode;
846+
use crate::aggregates::clustered_test_util::collect_under_limit;
846847
use crate::aggregates::{AggregateMode, PhysicalGroupBy};
847848
use crate::common::collect;
848849
use crate::execution_plan::ExecutionPlan;
@@ -1691,21 +1692,17 @@ mod tests {
16911692
aggregate.group_clustering_mode(),
16921693
&GroupClusteringMode::None
16931694
);
1694-
let context =
1695-
Arc::new(
1696-
TaskContext::default()
1697-
.with_session_config(SessionConfig::new().set_bool(
1698-
"datafusion.execution.enable_migration_aggregate",
1699-
true,
1700-
))
1701-
.with_runtime(
1702-
RuntimeEnvBuilder::new()
1703-
.with_memory_pool(Arc::new(GreedyMemoryPool::new(64 * 1024)))
1704-
.build_arc()?,
1705-
),
1706-
);
1707-
let output = collect(aggregate.execute(0, context)?).await?;
1695+
let output = collect_under_limit(&aggregate, 64 * 1024).await?;
17081696
assert_eq!(output.iter().map(|b| b.num_rows()).sum::<usize>(), 24576);
1697+
assert!(
1698+
aggregate
1699+
.metrics()
1700+
.unwrap()
1701+
.sum_by_name("early_emit_count")
1702+
.unwrap()
1703+
.as_usize()
1704+
> 0
1705+
);
17091706
Ok(())
17101707
}
17111708
}

0 commit comments

Comments
 (0)