CuteChuanChuan opened a new issue, #25333: URL: https://github.com/apache/datafusion/issues/25333
### Is your feature request related to a problem or challenge? Working on #25272 (issue: #25077) showed that join metrics can be wrong without any test failing. `probe_hit_rate` and `avg_fanout` of `HashJoinExec` were reported too low whenever a probe batch was split into several chunks. Before that PR, their values were only checked through `EXPLAIN ANALYZE` snapshots in `.slt` files (a Rust test only checked that they appear), on inputs small enough that no probe batch is ever split into chunks, and the Rust tests that do run with small `batch_size` values did not assert them. Other join metrics also have little or no test coverage, for example: - `left_input_rows` / `right_input_rows` of `SymmetricHashJoinExec` are not checked by any test - `build_input_rows` of `HashJoinExec` is not checked by any Rust test, only through `EXPLAIN ANALYZE` snapshots in `.slt` files ### Describe the solution you'd like Add metric tests for the join operators, one operator per PR: - `HashJoinExec` - `NestedLoopJoinExec` - `SortMergeJoinExec` - `SymmetricHashJoinExec` - `CrossJoinExec` - `PiecewiseMergeJoinExec` - `AsOfJoinExec` For each operator: 1. Check which of its metrics are already covered by existing tests. 2. Add tests with multi-batch inputs and small `batch_size` values, asserting metrics whose values should not depend on how the input is batched, such as row counts and ratio metrics like `probe_hit_rate`. The tests added in #25272 (`join_probe_metrics_count_each_probe_row_once` and `join_probe_metrics_count_probe_row_starting_new_chunk`) follow this approach for `HashJoinExec`, using the existing `hash_join_exec_configs` rstest matrix. If this sounds reasonable, I can turn this into a tracking issue with one sub-task per operator. ### Describe alternatives you've considered - **`EXPLAIN ANALYZE` cases in `.slt`** (with `set datafusion.execution.batch_size = ...`) instead of Rust unit tests on `MetricsSet`. These are closer to what users see, but non-deterministic values such as timings need `<slt:ignore>`, and running the same query over a matrix of configurations is less convenient. - **A shared helper** that runs any join plan at several `batch_size` values and compares metrics across runs, instead of per-operator tests. Less code per operator, but harder to express metrics that legitimately depend on batching, such as `output_batches`. Which style would maintainers prefer for this kind of test? ### Additional context Related: #25077, #25272 -- 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]
