mbutrovich opened a new pull request, #20936: URL: https://github.com/apache/datafusion/pull/20936
DataFusion Comet is still suffering from performance regressions in SMJ introduced with DF 52. We'd like to backport this fix from `main` to try to help. Benchmarks with Comet look to return a bit of SMJ performance back: **DF 52.3:** <img width="405" height="228" alt="Screenshot 2026-03-13 at 4 07 57 PM" src="https://github.com/user-attachments/assets/c418616c-6ed0-4598-87c0-a04c70f93d19"; /> **This PR** <img width="441" height="245" alt="Screenshot 2026-03-13 at 4 07 54 PM" src="https://github.com/user-attachments/assets/36849ce2-78e9-4e6f-8414-e7288b551731"; /> ## Which issue does this PR close? N/A - performance optimization ## Rationale for this change In the SMJ tight loop (`join_partial`), `num_unfrozen_pairs()` was called **twice per iteration**: once in the loop guard and once inside `append_output_pair`. This method iterates all chunks in `output_indices` and sums their lengths — O(num_chunks). Over a full batch of `batch_size` iterations, this makes the inner loop O(batch_size * num_chunks) instead of O(batch_size). ## What changes are included in this PR? Add a `num_output_rows` field to `StreamedBatch` that is incremented on each append and reset on freeze, replacing the O(n) summation with an O(1) field read. - Added `num_output_rows: usize` field to `StreamedBatch`, initialized to `0` - Increment `num_output_rows` in `append_output_pair()` after each append - `num_output_rows()` now returns the cached field directly - Reset to `0` in `freeze_streamed()` when `output_indices` is cleared - Removed the `num_unfrozen_pairs` parameter from `append_output_pair()` since it can now read `self.num_output_rows` directly ## Are these changes tested? Yes — all 48 existing `sort_merge_join` tests pass. This is a pure refactor of an internal counter with no behavioral change. ## Performance Very minor improvement. ### Before ``` sort_merge_join/inner_1to1/100000 time: [3.8146 ms 3.8229 ms 3.8314 ms] sort_merge_join/inner_1to10/100000 time: [16.094 ms 16.125 ms 16.161 ms] Found 7 outliers among 100 measurements (7.00%) 6 (6.00%) high mild 1 (1.00%) high severe sort_merge_join/left_1to1_unmatched/100000 time: [3.7823 ms 3.7861 ms 3.7902 ms] Found 4 outliers among 100 measurements (4.00%) 4 (4.00%) high mild sort_merge_join/left_semi_1to10/100000 time: [3.0523 ms 3.0755 ms 3.1023 ms] Found 14 outliers among 100 measurements (14.00%) 3 (3.00%) high mild 11 (11.00%) high severe sort_merge_join/left_anti_partial/100000 time: [3.3458 ms 3.3498 ms 3.3542 ms] Found 12 outliers among 100 measurements (12.00%) 8 (8.00%) high mild 4 (4.00%) high severe ``` ### After ``` sort_merge_join/inner_1to1/100000 time: [3.7162 ms 3.7207 ms 3.7254 ms] change: [−4.2320% −3.9309% −3.6431%] (p = 0.00 < 0.05) Performance has improved. Found 4 outliers among 100 measurements (4.00%) 4 (4.00%) high mild sort_merge_join/inner_1to10/100000 time: [15.556 ms 15.589 ms 15.626 ms] change: [−5.2786% −4.8329% −4.4351%] (p = 0.00 < 0.05) Performance has improved. Found 4 outliers among 100 measurements (4.00%) 1 (1.00%) high mild 3 (3.00%) high severe sort_merge_join/left_1to1_unmatched/100000 time: [3.7059 ms 3.7101 ms 3.7146 ms] change: [−4.4526% −4.1565% −3.8660%] (p = 0.00 < 0.05) Performance has improved. Found 2 outliers among 100 measurements (2.00%) 2 (2.00%) high mild sort_merge_join/left_semi_1to10/100000 time: [3.0832 ms 3.0899 ms 3.0981 ms] change: [−4.0965% −3.4158% −2.7657%] (p = 0.00 < 0.05) Performance has improved. Found 3 outliers among 100 measurements (3.00%) 1 (1.00%) high mild 2 (2.00%) high severe sort_merge_join/left_anti_partial/100000 time: [3.2963 ms 3.3048 ms 3.3153 ms] change: [−3.9413% −3.5316% −3.0884%] (p = 0.00 < 0.05) Performance has improved. Found 8 outliers among 100 measurements (8.00%) 3 (3.00%) high mild 5 (5.00%) high severe ``` ## Are there any user-facing changes? No. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- (cherry picked from commit 33b86fe02e7bbe63135995c2dbb47bf83c08143c) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> - Closes #. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> -- 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]
