alamb commented on code in PR #26562: URL: https://github.com/apache/datafusion/pull/26562#discussion_r4364360388
########## datafusion/physical-plan/src/aggregates/snapshots/datafusion_physical_plan__aggregates__memory_call_sequence_tests__final_spills_twice_then_repl...@ctrl16.snap: ########## @@ -0,0 +1,45 @@ +--- +source: datafusion/physical-plan/src/aggregates/memory_call_sequence_tests.rs Review Comment: I recommend making this an inline snapshot rather than put it in its own file as I find such structure easier to see as the test and the expected output are next to each other ########## datafusion/physical-plan/src/aggregates/mod.rs: ########## @@ -221,6 +221,8 @@ pub mod group_values; mod grouped_hash_stream; mod grouped_topk_stream; mod hash_stream; +#[cfg(test)] +mod memory_call_sequence_tests; Review Comment: Rather than a separate module, I recommend putting this in its own test binary (maybe is there a `tests/physical-plan-integration.rs` location to follow the model in other crates? ########## datafusion/physical-plan/src/aggregates/snapshots/datafusion_physical_plan__aggregates__memory_call_sequence_tests__final_spills_twice_then_repl...@ctrl16.snap: ########## @@ -0,0 +1,45 @@ +--- +source: datafusion/physical-plan/src/aggregates/memory_call_sequence_tests.rs +expression: snapshot +--- +FinalHashAggregateStream[0] register can_spill=true Review Comment: this is so great to be able to visually see what the call chain is (and we will be able to more easily understand what changes if the behavior changes) -- 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]
