Skip to content

Improve test coverage for join operator metrics #25333

Description

@CuteChuanChuan

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

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions