chore: refactor `BuildProbeJoinMetrics` to use `BaselineMetrics` by Samyak2 · Pull Request #16500 · apache/datafusion

Samyak2 · 2025-06-22T07:13:45Z

Which issue does this PR close?

Closes #16495

Rationale for this change

What changes are included in this PR?

Add a BaselineMetrics and remove output_rows from BuildProbeJoinMetrics.

The elapsed_compute in baseline is populated in the Drop trait implementation by summing up join_time and build_time.

Are these changes tested?

Here's an example of an explain analyze of a hash join showing these metrics:

[(WatchID@0, WatchID@0)], metrics=[output_rows=100, elapsed_compute=2.313624ms, build_input_batches=1, build_input_rows=100, input_batches=1, input_rows=100, output_batches=1, build_mem_used=3688, build_time=865.832µs, join_time=1.369875ms]

Notice output_rows=100, elapsed_compute=2.313624ms in the above.

Are there any user-facing changes?

No

2010YOUY01

Thank you! I left some suggestions, looking forward to your thoughts.

datafusion/physical-plan/src/joins/cross_join.rs

2010YOUY01 · 2025-06-24T08:14:25Z

datafusion/physical-plan/src/joins/utils.rs

-    pub(crate) output_rows: metrics::Count,
+}
+
+impl Drop for BuildProbeJoinMetrics {


I don't think it's the best way but I guess it's okay to merge 🤔 If we forget to count a specific time period in some join operator, we can fix it in the future.

Could you also add a brief comment to explain this drop?

I agree. This is not intuitive at all. When I came back to this PR to act on your suggestions, it took me a while to remember how this works :)

I have added a comment for now. I would be open to other ways of handling this!

Samyak2 · 2025-06-28T10:19:06Z

Rebased on latest main

2010YOUY01 · 2025-06-29T05:16:52Z

Thank you! this implementation looks correct to me.

Since the state transition in joins are tricky, could you add a test (or ensure there are some existing tests), to double-check this refactor won't change the result of the related metrics?

Closes apache#16495 Here's an example of an `explain analyze` of a hash join showing these metrics: ``` [(WatchID@0, WatchID@0)], metrics=[output_rows=100, elapsed_compute=2.313624ms, build_input_batches=1, build_input_rows=100, input_batches=1, input_rows=100, output_batches=1, build_mem_used=3688, build_time=865.832µs, join_time=1.369875ms] ``` Notice `output_rows=100, elapsed_compute=2.313624ms` in the above.

Samyak2 · 2025-07-05T18:28:32Z

I have added asserts for metrics in the existing join tests. The ones in hash_join and cross_join are working. The asserts in nested_loop_join are currently failing due to a mismatch in output_rows. I'm debugging this, but I have put this out here in case anyone has an idea about this (or if this is expected):

failures:

---- joins::nested_loop_join::tests::join_left_anti_with_filter stdout ----

thread 'joins::nested_loop_join::tests::join_left_anti_with_filter' panicked at datafusion/physical-plan/src/joins/nested_loop_join.rs:1405:9:
assertion `left == right` failed
  left: 0
 right: 2
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- joins::nested_loop_join::tests::join_left_semi_with_filter stdout ----

thread 'joins::nested_loop_join::tests::join_left_semi_with_filter' panicked at datafusion/physical-plan/src/joins/nested_loop_join.rs:1375:9:
assertion `left == right` failed
  left: 0
 right: 1

---- joins::nested_loop_join::tests::join_left_with_filter stdout ----

thread 'joins::nested_loop_join::tests::join_left_with_filter' panicked at datafusion/physical-plan/src/joins/nested_loop_join.rs:1282:9:
assertion `left == right` failed
  left: 1
 right: 3

---- joins::nested_loop_join::tests::join_full_with_filter stdout ----

thread 'joins::nested_loop_join::tests::join_full_with_filter' panicked at datafusion/physical-plan/src/joins/nested_loop_join.rs:1346:9:
assertion `left == right` failed
  left: 3
 right: 5

---- joins::nested_loop_join::tests::join_left_mark_with_filter stdout ----

thread 'joins::nested_loop_join::tests::join_left_mark_with_filter' panicked at datafusion/physical-plan/src/joins/nested_loop_join.rs:1495:9:
assertion `left == right` failed
  left: 0
 right: 3

Samyak2 · 2025-07-05T18:46:41Z

Actually, I see the same behavior on latest main. Looks like the output_rows metric in nested loop join is currently wrong?

datafusion/physical-plan/src/joins/nested_loop_join.rs

2010YOUY01 · 2025-07-06T03:41:08Z

Actually, I see the same behavior on latest main. Looks like the output_rows metric in nested loop join is currently wrong?

Thank you for the catch! Here might also need record_poll(): https://github.com/apache/datafusion/pull/16500/files#r2187952197

Samyak2 · 2025-07-06T05:27:52Z

Actually, I see the same behavior on latest main. Looks like the output_rows metric in nested loop join is currently wrong?

Thank you for the catch! Here might also need record_poll(): https://github.com/apache/datafusion/pull/16500/files#r2187952197

Thanks for the pointer! It makes sense now. I have added a record_poll there and also updated the output_batches metric inside the function. Please take a look

This was needed because ExhaustedProbeSide state can also return output rows - in certain types of joins. Without this, the output_rows metric for nested loop join was wrong!

Samyak2 · 2025-07-06T09:13:16Z

Fixed the formatting issues

2010YOUY01

Thanks again.

alamb · 2025-07-07T11:27:44Z

Thanks @Samyak2 and @2010YOUY01

github-actions bot added the physical-plan Changes to the physical-plan crate label Jun 22, 2025

2010YOUY01 reviewed Jun 24, 2025

View reviewed changes

Samyak2 force-pushed the issue-16495 branch 2 times, most recently from 6fb44a6 to b5e87fb Compare June 28, 2025 10:18

Samyak2 force-pushed the issue-16495 branch from b5e87fb to 033ac64 Compare July 5, 2025 18:24

2010YOUY01 reviewed Jul 6, 2025

View reviewed changes

datafusion/physical-plan/src/joins/nested_loop_join.rs Show resolved Hide resolved

Samyak2 force-pushed the issue-16495 branch from 23a3fd8 to 9b3d833 Compare July 6, 2025 06:42

Samyak2 added 2 commits July 6, 2025 14:42

test: add checks for join metrics in tests

450fed2

fix: add record_poll to ExhaustedProbeSide for nested_loop_join

008c962

This was needed because ExhaustedProbeSide state can also return output rows - in certain types of joins. Without this, the output_rows metric for nested loop join was wrong!

Samyak2 force-pushed the issue-16495 branch from 9b3d833 to 008c962 Compare July 6, 2025 09:12

2010YOUY01 approved these changes Jul 6, 2025

View reviewed changes

alamb merged commit 01698cb into apache:main Jul 7, 2025
27 checks passed

2010YOUY01 mentioned this pull request Jul 9, 2025

limit intermediate batch size in nested_loop_join #16443

Merged

Conversation

Samyak2 commented Jun 22, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

2010YOUY01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

2010YOUY01 Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Samyak2 Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Samyak2 commented Jun 28, 2025

Uh oh!

2010YOUY01 commented Jun 29, 2025

Uh oh!

Samyak2 commented Jul 5, 2025

Uh oh!

Samyak2 commented Jul 5, 2025

Uh oh!

Uh oh!

2010YOUY01 commented Jul 6, 2025

Uh oh!

Samyak2 commented Jul 6, 2025

Uh oh!

Samyak2 commented Jul 6, 2025

Uh oh!

2010YOUY01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Jul 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants