@@ -39,10 +39,12 @@ use datafusion_execution::{SendableRecordBatchStream, TaskContext};
3939use datafusion_functions_aggregate:: array_agg:: array_agg_udaf;
4040use datafusion_physical_expr:: aggregate:: AggregateExprBuilder ;
4141use datafusion_physical_expr:: expressions:: { Column , col} ;
42+ use datafusion_physical_expr_common:: metrics:: MetricsSet ;
4243use datafusion_physical_expr_common:: sort_expr:: LexOrdering ;
4344use datafusion_physical_plan:: aggregates:: {
4445 AggregateExec , AggregateMode , PhysicalGroupBy ,
4546} ;
47+ use datafusion_physical_plan:: metrics:: MetricValue ;
4648use datafusion_physical_plan:: stream:: RecordBatchStreamAdapter ;
4749use futures:: StreamExt ;
4850
@@ -69,16 +71,18 @@ async fn test_sort_with_limited_memory() -> Result<()> {
6971
7072 // Basic test with a lot of groups that cannot all fit in memory and 1 record batch
7173 // from each spill file is too much memory
72- let spill_count = run_sort_test_with_limited_memory ( RunTestWithLimitedMemoryArgs {
74+ let metrics = run_sort_test_with_limited_memory ( RunTestWithLimitedMemoryArgs {
7375 pool_size,
7476 task_ctx : Arc :: new ( task_ctx) ,
7577 number_of_record_batches : 100 ,
7678 get_size_of_record_batch_to_generate : Box :: pin ( move |_| record_batch_size) ,
7779 memory_behavior : Default :: default ( ) ,
80+ assert_all_output_batches_roughly_match_batch_size_conf : true ,
7881 } )
7982 . await ?;
8083
81- let total_spill_files_size = spill_count * record_batch_size;
84+ let total_spill_files_size =
85+ metrics. spill_count ( ) . unwrap_or_default ( ) * record_batch_size;
8286 assert ! (
8387 total_spill_files_size > pool_size,
8488 "Total spill files size {total_spill_files_size} should be greater than pool size {pool_size}" ,
@@ -119,6 +123,7 @@ async fn test_sort_with_limited_memory_and_different_sizes_of_record_batch() ->
119123 }
120124 } ) ,
121125 memory_behavior : Default :: default ( ) ,
126+ assert_all_output_batches_roughly_match_batch_size_conf : true ,
122127 } )
123128 . await ?;
124129
@@ -157,6 +162,7 @@ async fn test_sort_with_limited_memory_and_different_sizes_of_record_batch_and_c
157162 }
158163 } ) ,
159164 memory_behavior : MemoryBehavior :: TakeAllMemoryAndReleaseEveryNthBatch ( 10 ) ,
165+ assert_all_output_batches_roughly_match_batch_size_conf : true ,
160166 } )
161167 . await ?;
162168
@@ -195,6 +201,7 @@ async fn test_sort_with_limited_memory_and_different_sizes_of_record_batch_and_t
195201 }
196202 } ) ,
197203 memory_behavior : MemoryBehavior :: TakeAllMemoryAtTheBeginning ,
204+ assert_all_output_batches_roughly_match_batch_size_conf : true ,
198205 } )
199206 . await ?;
200207
@@ -227,6 +234,7 @@ async fn test_sort_with_limited_memory_and_large_record_batch() -> Result<()> {
227234 number_of_record_batches : 100 ,
228235 get_size_of_record_batch_to_generate : Box :: pin ( move |_| pool_size / 6 ) ,
229236 memory_behavior : Default :: default ( ) ,
237+ assert_all_output_batches_roughly_match_batch_size_conf : true ,
230238 } )
231239 . await ?;
232240
@@ -252,21 +260,33 @@ async fn test_sort_with_limited_memory_and_oversized_record_batch() -> Result<()
252260 ) )
253261 } ;
254262
263+ let number_of_record_batches = 100 ;
264+
255265 // Each spilled run's largest batch is so big that two merge streams cannot be
256266 // reserved at once even at the smallest read-buffer size (`2 * (2 * batch) >
257267 // pool`), yet a single stream still fits (`2 * batch < pool`). Reducing the
258268 // buffer size therefore cannot help, the multi-level merge has to re-spill a
259269 // run with a smaller batch size to make progress instead of failing with
260270 // `ResourcesExhausted`.
261- run_sort_test_with_limited_memory ( RunTestWithLimitedMemoryArgs {
271+ let metrics = run_sort_test_with_limited_memory ( RunTestWithLimitedMemoryArgs {
262272 pool_size,
263273 task_ctx : Arc :: new ( task_ctx) ,
264- number_of_record_batches : 100 ,
274+ number_of_record_batches,
265275 get_size_of_record_batch_to_generate : Box :: pin ( move |_| pool_size / 3 ) ,
266276 memory_behavior : Default :: default ( ) ,
277+
278+ assert_all_output_batches_roughly_match_batch_size_conf : false ,
267279 } )
268280 . await ?;
269281
282+ let output_batches = get_output_batches_from_metrics ( & metrics) ;
283+
284+ // minimum 2 batches more
285+ assert ! (
286+ output_batches >= number_of_record_batches + 2 ,
287+ "output_batches {output_batches} should be greater than number_of_record_batches ({number_of_record_batches}) + 2"
288+ ) ;
289+
270290 Ok ( ( ) )
271291}
272292
@@ -277,6 +297,9 @@ struct RunTestWithLimitedMemoryArgs {
277297 get_size_of_record_batch_to_generate :
278298 Pin < Box < dyn Fn ( usize ) -> usize + Send + ' static > > ,
279299 memory_behavior : MemoryBehavior ,
300+
301+ /// When true we would `assert_eq(the number of output_rows metric / output_batches metric == task_ctx.batch_size)`
302+ assert_all_output_batches_roughly_match_batch_size_conf : bool ,
280303}
281304
282305#[ derive( Default ) ]
@@ -289,7 +312,7 @@ enum MemoryBehavior {
289312
290313async fn run_sort_test_with_limited_memory (
291314 mut args : RunTestWithLimitedMemoryArgs ,
292- ) -> Result < usize > {
315+ ) -> Result < MetricsSet > {
293316 let get_size_of_record_batch_to_generate = std:: mem:: replace (
294317 & mut args. get_size_of_record_batch_to_generate ,
295318 Box :: pin ( move |_| unreachable ! ( "should not be called after take" ) ) ,
@@ -349,7 +372,23 @@ async fn run_sort_test_with_limited_memory(
349372
350373 let result = sort_exec. execute ( 0 , Arc :: clone ( & args. task_ctx ) ) ?;
351374
352- run_test ( args, sort_exec, result) . await
375+ let number_of_record_batches = args. number_of_record_batches ;
376+ let assert_output_batch_size =
377+ args. assert_all_output_batches_roughly_match_batch_size_conf ;
378+
379+ let metrics = run_test ( args, sort_exec, result) . await ?;
380+
381+ assert_baseline_metrics_for_non_empty_output (
382+ & metrics,
383+ number_of_record_batches * record_batch_size as usize ,
384+ if assert_output_batch_size {
385+ Some ( record_batch_size as usize )
386+ } else {
387+ None
388+ } ,
389+ ) ;
390+
391+ Ok ( metrics)
353392}
354393
355394fn grow_memory_as_much_as_possible (
@@ -383,17 +422,19 @@ async fn test_aggregate_with_high_cardinality_with_limited_memory() -> Result<()
383422
384423 // Basic test with a lot of groups that cannot all fit in memory and 1 record batch
385424 // from each spill file is too much memory
386- let spill_count =
425+ let metrics =
387426 run_test_aggregate_with_high_cardinality ( RunTestWithLimitedMemoryArgs {
388427 pool_size,
389428 task_ctx : Arc :: new ( task_ctx) ,
390429 number_of_record_batches : 100 ,
391430 get_size_of_record_batch_to_generate : Box :: pin ( move |_| record_batch_size) ,
392431 memory_behavior : Default :: default ( ) ,
432+ assert_all_output_batches_roughly_match_batch_size_conf : true ,
393433 } )
394434 . await ?;
395435
396- let total_spill_files_size = spill_count * record_batch_size;
436+ let total_spill_files_size =
437+ metrics. spill_count ( ) . unwrap_or_default ( ) * record_batch_size;
397438 assert ! (
398439 total_spill_files_size > pool_size,
399440 "Total spill files size {total_spill_files_size} should be greater than pool size {pool_size}" ,
@@ -430,6 +471,7 @@ async fn test_aggregate_with_high_cardinality_with_limited_memory_and_different_
430471 }
431472 } ) ,
432473 memory_behavior : Default :: default ( ) ,
474+ assert_all_output_batches_roughly_match_batch_size_conf : true ,
433475 } )
434476 . await ?;
435477
@@ -464,6 +506,7 @@ async fn test_aggregate_with_high_cardinality_with_limited_memory_and_different_
464506 }
465507 } ) ,
466508 memory_behavior : MemoryBehavior :: TakeAllMemoryAndReleaseEveryNthBatch ( 10 ) ,
509+ assert_all_output_batches_roughly_match_batch_size_conf : true ,
467510 } )
468511 . await ?;
469512
@@ -498,6 +541,7 @@ async fn test_aggregate_with_high_cardinality_with_limited_memory_and_different_
498541 }
499542 } ) ,
500543 memory_behavior : MemoryBehavior :: TakeAllMemoryAtTheBeginning ,
544+ assert_all_output_batches_roughly_match_batch_size_conf : true ,
501545 } )
502546 . await ?;
503547
@@ -527,6 +571,7 @@ async fn test_aggregate_with_high_cardinality_with_limited_memory_and_large_reco
527571 number_of_record_batches : 100 ,
528572 get_size_of_record_batch_to_generate : Box :: pin ( move |_| pool_size / 6 ) ,
529573 memory_behavior : Default :: default ( ) ,
574+ assert_all_output_batches_roughly_match_batch_size_conf : true ,
530575 } )
531576 . await ?;
532577
@@ -535,7 +580,7 @@ async fn test_aggregate_with_high_cardinality_with_limited_memory_and_large_reco
535580
536581async fn run_test_aggregate_with_high_cardinality (
537582 mut args : RunTestWithLimitedMemoryArgs ,
538- ) -> Result < usize > {
583+ ) -> Result < MetricsSet > {
539584 let get_size_of_record_batch_to_generate = std:: mem:: replace (
540585 & mut args. get_size_of_record_batch_to_generate ,
541586 Box :: pin ( move |_| unreachable ! ( "should not be called after take" ) ) ,
@@ -624,20 +669,21 @@ async fn run_test(
624669 args : RunTestWithLimitedMemoryArgs ,
625670 plan : Arc < dyn ExecutionPlan > ,
626671 result_stream : SendableRecordBatchStream ,
627- ) -> Result < usize > {
672+ ) -> Result < MetricsSet > {
628673 let number_of_record_batches = args. number_of_record_batches ;
629674
630675 consume_stream_and_simulate_other_running_memory_consumers ( args, result_stream)
631676 . await ?;
632677
678+ let metrics = plan. metrics ( ) . expect ( "must have metrics" ) ;
633679 let spill_count = assert_spill_count_metric ( true , plan) ;
634680
635681 assert ! (
636682 spill_count > 0 ,
637683 "Expected spill, but did not, number of record batches: {number_of_record_batches}" ,
638684 ) ;
639685
640- Ok ( spill_count )
686+ Ok ( metrics )
641687}
642688
643689/// Consume the stream and change the amount of memory used while consuming it based on the [`MemoryBehavior`] provided
@@ -693,3 +739,56 @@ async fn consume_stream_and_simulate_other_running_memory_consumers(
693739
694740 Ok ( ( ) )
695741}
742+
743+ /// Assert baseline metrics are as expected or around that
744+ ///
745+ /// `output_batch_size` should be `None` when you expect to not get batched at the same size
746+ /// `Some(session conf batch size)` for the rest
747+ fn assert_baseline_metrics_for_non_empty_output (
748+ metrics : & MetricsSet ,
749+ expected_output_rows : usize ,
750+ output_batch_size : Option < usize > ,
751+ ) {
752+ let end_time = metrics
753+ . iter ( )
754+ . find_map ( |item| match item. value ( ) {
755+ MetricValue :: EndTimestamp ( end) => Some ( end) ,
756+ _ => None ,
757+ } )
758+ . expect ( "Must have end time metric since it exists in the baseline" ) ;
759+
760+ assert_ne ! ( end_time. value( ) , None ) ;
761+
762+ assert_eq ! ( metrics. output_rows( ) , Some ( expected_output_rows) ) ;
763+
764+ let output_bytes = metrics
765+ . iter ( )
766+ . find_map ( |item| match item. value ( ) {
767+ MetricValue :: OutputBytes ( total) => Some ( total) ,
768+ _ => None ,
769+ } )
770+ . expect ( "Must have output_bytes metric since it exists in the baseline" ) ;
771+
772+ assert_ne ! ( output_bytes. value( ) , 0_usize ) ;
773+
774+ let output_batches = get_output_batches_from_metrics ( metrics) ;
775+
776+ if let Some ( output_batch_size) = output_batch_size {
777+ assert_eq ! (
778+ output_batches,
779+ expected_output_rows. div_ceil( output_batch_size)
780+ ) ;
781+ } else {
782+ assert_ne ! ( output_batches, 0 , ) ;
783+ }
784+ }
785+
786+ fn get_output_batches_from_metrics ( metrics : & MetricsSet ) -> usize {
787+ metrics
788+ . iter ( )
789+ . find_map ( |item| match item. value ( ) {
790+ MetricValue :: OutputBatches ( total) => Some ( total. value ( ) ) ,
791+ _ => None ,
792+ } )
793+ . expect ( "Must have output_batches metric since it exists in the baseline" )
794+ }
0 commit comments