Skip to content

Commit 2ba6d84

Browse files
committed
fix: restore upstream aggregates changes dropped in the rebase
Resolving the rebase conflict in `aggregates/mod.rs` took this branch's copy of the whole file, which reverted the changes made upstream while the branch was in flight: - `constant_grouping_expr_is_not_a_completion_boundary` and the imports it needs, added with the constant-boundary fix (#25019). That fix's non-test half moved into the builder with the rest of the construction logic, so only the test was missing. - the `test_grouped_aggregation_respects_memory_limit` assertion, which upstream relaxed to match on `e.find_root()`. Aggregate spilling reports a `ResourcesExhausted` wrapped in a `Context`, which the older assertion could not see through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwTc51ca2XHDCyVB7MbJoz
1 parent 0f238b0 commit 2ba6d84

1 file changed

Lines changed: 88 additions & 4 deletions

File tree

  • datafusion/physical-plan/src/aggregates

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

Lines changed: 88 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3198,7 +3198,7 @@ mod tests {
31983198

31993199
use arrow::array::{
32003200
BooleanArray, DictionaryArray, Float32Array, Float64Array, Int32Array,
3201-
Int64Array, NullArray, StructArray, UInt32Array, UInt64Array,
3201+
Int64Array, NullArray, StringArray, StructArray, UInt32Array, UInt64Array,
32023202
};
32033203
use arrow::compute::{SortOptions, concat_batches};
32043204
use arrow::datatypes::Int32Type;
@@ -3209,7 +3209,7 @@ mod tests {
32093209
use datafusion_execution::runtime_env::RuntimeEnvBuilder;
32103210
use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs};
32113211
use datafusion_expr::{
3212-
Accumulator, AggregateUDF, AggregateUDFImpl, EmitTo, GroupsAccumulator,
3212+
Accumulator, AggregateUDF, AggregateUDFImpl, EmitTo, GroupsAccumulator, Operator,
32133213
Signature, Volatility,
32143214
};
32153215
use datafusion_functions_aggregate::approx_percentile_cont::approx_percentile_cont_udaf;
@@ -3223,7 +3223,7 @@ mod tests {
32233223
use datafusion_physical_expr::Partitioning;
32243224
use datafusion_physical_expr::PhysicalSortExpr;
32253225
use datafusion_physical_expr::aggregate::AggregateExprBuilder;
3226-
use datafusion_physical_expr::expressions::{Literal, NotExpr};
3226+
use datafusion_physical_expr::expressions::{Literal, NotExpr, binary};
32273227

32283228
use crate::projection::ProjectionExec;
32293229
use crate::repartition::RepartitionExec;
@@ -6444,6 +6444,87 @@ mod tests {
64446444
Ok(())
64456445
}
64466446

6447+
#[tokio::test]
6448+
async fn constant_grouping_expr_is_not_a_completion_boundary() -> Result<()> {
6449+
let schema = Arc::new(Schema::new(vec![
6450+
Field::new("key", DataType::Int32, true),
6451+
Field::new("market", DataType::Utf8, true),
6452+
Field::new("value", DataType::Int64, false),
6453+
]));
6454+
let batch = RecordBatch::try_new(
6455+
Arc::clone(&schema),
6456+
vec![
6457+
Arc::new(Int32Array::from(vec![None, Some(10), Some(10)])),
6458+
Arc::new(StringArray::from(vec![Some("US"), Some("US"), Some("US")])),
6459+
Arc::new(Int64Array::from(vec![3, 1, 2])),
6460+
],
6461+
)?;
6462+
6463+
let build_aggregate = |input: Arc<dyn ExecutionPlan>| -> Result<AggregateExec> {
6464+
let predicate =
6465+
binary(col("market", &schema)?, Operator::Eq, lit("US"), &schema)?;
6466+
let input = Arc::new(FilterExecBuilder::new(predicate, input).build()?);
6467+
AggregateExec::try_new(
6468+
AggregateMode::Single,
6469+
PhysicalGroupBy::new_single(vec![
6470+
(col("key", &schema)?, "key".to_string()),
6471+
(col("market", &schema)?, "market".to_string()),
6472+
]),
6473+
vec![Arc::new(
6474+
AggregateExprBuilder::new(count_udaf(), vec![col("value", &schema)?])
6475+
.schema(Arc::clone(&schema))
6476+
.alias("COUNT(value)")
6477+
.build()?,
6478+
)],
6479+
vec![None],
6480+
input,
6481+
Arc::clone(&schema),
6482+
)
6483+
};
6484+
6485+
let unordered_input = TestMemoryExec::try_new_exec(
6486+
&[vec![batch.clone()]],
6487+
Arc::clone(&schema),
6488+
None,
6489+
)?;
6490+
let aggregate = build_aggregate(unordered_input)?;
6491+
assert_eq!(aggregate.input_order_mode(), &InputOrderMode::Linear);
6492+
assert_eq!(
6493+
aggregate.schema().as_ref(),
6494+
&Schema::new(vec![
6495+
Field::new("key", DataType::Int32, true),
6496+
Field::new("market", DataType::Utf8, true),
6497+
Field::new("COUNT(value)", DataType::Int64, false),
6498+
])
6499+
);
6500+
6501+
let output =
6502+
collect(aggregate.execute(0, Arc::new(TaskContext::default()))?).await?;
6503+
assert_eq!(output.iter().map(RecordBatch::num_rows).sum::<usize>(), 2);
6504+
assert_snapshot!(batches_to_sort_string(&output), @r"
6505+
+-----+--------+--------------+
6506+
| key | market | COUNT(value) |
6507+
+-----+--------+--------------+
6508+
| | US | 1 |
6509+
| 10 | US | 2 |
6510+
+-----+--------+--------------+
6511+
");
6512+
6513+
let ordering = LexOrdering::new([PhysicalSortExpr::new_default(Arc::new(
6514+
Column::new("key", 0),
6515+
))])
6516+
.unwrap();
6517+
let ordered_input =
6518+
TestMemoryExec::try_new(&[vec![batch]], Arc::clone(&schema), None)?
6519+
.try_with_sort_information(vec![ordering])?;
6520+
let ordered_input =
6521+
Arc::new(TestMemoryExec::update_cache(&Arc::new(ordered_input)));
6522+
let aggregate = build_aggregate(ordered_input)?;
6523+
assert_eq!(aggregate.input_order_mode(), &InputOrderMode::Sorted);
6524+
6525+
Ok(())
6526+
}
6527+
64476528
#[tokio::test]
64486529
async fn test_grouped_aggregation_respects_memory_limit() -> Result<()> {
64496530
// test with spill
@@ -6546,7 +6627,10 @@ mod tests {
65466627
");
65476628
}
65486629
}
6549-
Err(e) => assert!(matches!(e, DataFusionError::ResourcesExhausted(_))),
6630+
Err(e) => assert!(
6631+
matches!(e.find_root(), DataFusionError::ResourcesExhausted(_)),
6632+
"unexpected error: {e}"
6633+
),
65506634
}
65516635

65526636
Ok(())

0 commit comments

Comments
 (0)