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:
- Check which of its metrics are already covered by existing tests.
- 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
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_rateandavg_fanoutofHashJoinExecwere reported too low whenever a probe batch was split into several chunks. Before that PR, their values were only checked throughEXPLAIN ANALYZEsnapshots in.sltfiles (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 smallbatch_sizevalues did not assert them.Other join metrics also have little or no test coverage, for example:
left_input_rows/right_input_rowsofSymmetricHashJoinExecare not checked by any testbuild_input_rowsofHashJoinExecis not checked by any Rust test, only throughEXPLAIN ANALYZEsnapshots in.sltfilesDescribe the solution you'd like
Add metric tests for the join operators, one operator per PR:
HashJoinExecNestedLoopJoinExecSortMergeJoinExecSymmetricHashJoinExecCrossJoinExecPiecewiseMergeJoinExecAsOfJoinExecFor each operator:
batch_sizevalues, asserting metrics whose values should not depend on how the input is batched, such as row counts and ratio metrics likeprobe_hit_rate.The tests added in #25272 (
join_probe_metrics_count_each_probe_row_onceandjoin_probe_metrics_count_probe_row_starting_new_chunk) follow this approach forHashJoinExec, using the existinghash_join_exec_configsrstest 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 ANALYZEcases in.slt(withset datafusion.execution.batch_size = ...) instead of Rust unit tests onMetricsSet. 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.batch_sizevalues 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 asoutput_batches.Which style would maintainers prefer for this kind of test?
Additional context
Related: #25077, #25272