Skip to content

fix: record join_time and output metrics in PiecewiseMergeJoin - #24689

Open
buraksenn wants to merge 1 commit into
apache:mainfrom
buraksenn:fix-pwmj-classic-metrics
Open

fix: record join_time and output metrics in PiecewiseMergeJoin#24689
buraksenn wants to merge 1 commit into
apache:mainfrom
buraksenn:fix-pwmj-classic-metrics

Conversation

@buraksenn

@buraksenn buraksenn commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • Closes N/A

Rationale for this change

EXPLAIN ANALYZE on a PiecewiseMergeJoin always reports join_time=0 and, for classic joins, output_rows=0 / output_bytes=0.0 B, so PWMJ plans can't be profiled like other joins.

What changes are included in this PR?

  • Classic stream: time probe-phase work under join_time (started only after the streamed input's poll returns Ready) and record output metrics via record_poll
  • Existence stream: same join_time coverage for the scan and final pass
  • elapsed_compute picks both up via the existing BuildProbeJoinMetrics drop

Are these changes tested?

Yes. Classic tests now assert assert_join_metrics!, the existence final-pass test pins join_time > 0, and a new test verifies join_time excludes streamed-input wait. explain_analyze.slt shows the real output_bytes.

Are there any user-facing changes?

No (PWMJ is experimental and off by default).

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Aug 26, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.65363% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.45%. Comparing base (e2e94ae) to head (f08cd43).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...lan/src/joins/piecewise_merge_join/classic_join.rs 75.90% 28 Missing and 12 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24689      +/-   ##
==========================================
- Coverage   81.45%   81.45%   -0.01%     
==========================================
  Files        1119     1119              
  Lines      400030   400182     +152     
  Branches   400030   400182     +152     
==========================================
+ Hits       325850   325949      +99     
- Misses      55144    55179      +35     
- Partials    19036    19054      +18     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jayzhan211 jayzhan211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @buraksenn, LGTM

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

Labels

physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants