Sunninsky opened a new pull request, #27355:
URL: https://github.com/apache/datafusion/pull/27355

   ## Which issue does this PR close?
   
   - Part of #23805 (the 54.2.0 patch release tracking issue).
   - Backport of #24573 to `branch-54`.
   
   ## Rationale for this change
   
   `LEFT`, `RIGHT` and `FULL` sort-merge joins with a join filter can return 
rows out of order. These join types advertise that they preserve the order of 
one input (`maintains_input_order` is `[true, false]` for `LEFT`), so an 
operator above is allowed to rely on it. Deferred-filtered joins stage their 
output in a second `BatchCoalescer` (`self.output`), but in the `Exhausted` 
state `poll_next` returned the final filtered batch directly, so rows still 
buffered in `output` from an earlier flush came out *after* it.
   
   The three streamed-order tests from #24573 fail on `branch-54` as they do on 
`main`: for the `LEFT JOIN` shape with mixed group sizes the streamed keys come 
back as `[5, 6, 0, 1, 2, 3, 4]` instead of `[0, 1, 2, 3, 4, 5, 6]`. This is a 
wrong result, which makes it eligible for backport.
   
   #24573 fixed it on `main` (labelled `v56.0.0`); it is on `branch-56` but not 
on `branch-54`. It was requested for `branch-54` on #26868 
(https://github.com/apache/datafusion/issues/26868#issuecomment-6813785483), 
"to prevent regressions on upgrade to 54".
   
   ## What changes are included in this PR?
   
   This is a partial port, not a plain cherry-pick. The sort-merge join on this 
branch predates the generator refactor (#23761, #23976), and a cherry-pick of 
#24573 conflicts in nine places in `materializing_stream.rs` (and once in 
`tests.rs`) because most of that commit refactors code that is different here. 
Only the ordering fix and its tests are ported:
   
   - 
`datafusion/physical-plan/src/joins/sort_merge_join/materializing_stream.rs`: 
in the `Exhausted` state the final filtered batch is pushed into `self.output` 
instead of being returned directly. The existing drain of `self.output` below 
it then emits it, in order, after the rows that were already staged there.
   - `datafusion/physical-plan/src/joins/sort_merge_join/tests.rs`: 
`left_join_with_filter_preserves_streamed_order`, 
`right_join_with_filter_preserves_streamed_order` and 
`left_join_with_partial_filter_preserves_streamed_order`, with their helpers, 
taken from #24573.
   
   Not ported, because they are refactors or performance work on code that is 
different here and the fix does not need them: `join_arrays` returning 
`Result`, the direct-addressed source lookup in `materialize_right_columns` and 
its two tests, `new_output_coalescer`, and the benchmark change.
   
   ## Are these changes tested?
   
   Yes. On this branch (Rust 1.95.0) I ran:
   
   - With the three tests added and no fix, `cargo test -p 
datafusion-physical-plan --lib sort_merge_join` fails exactly those three.
   - With the fix, `cargo test -p datafusion-physical-plan --lib`: 1442 passed, 
including the three.
   - `cargo test -p datafusion-sqllogictest --test sqllogictests -- join`: all 
14 matching files pass.
   - `cargo fmt --all -- --check` and `cargo clippy -p datafusion-physical-plan 
--all-targets --all-features -- -D warnings` pass.
   
   ## Are there any user-facing changes?
   
   Outer sort-merge joins with a join filter return rows in the order they 
advertise. No public API change.
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
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]

Reply via email to