comphead commented on PR #26277: URL: https://github.com/apache/datafusion/pull/26277#issuecomment-6157493572
Thanks for the PR. The change reads correct to me, and keeping the one-row path inline while moving the bulk path out of line makes sense. One question on test coverage. The description cites `join_fuzz` for the bulk path, but I think the fuzz inputs rarely produce a run of more than one row. `make_staggered_batches_i32` draws `(a, b)` from `0..100` x `0..100` for 1000 rows, and `on_columns()` joins on both columns. That is about 10,000 distinct keys for 1000 rows, so most key groups hold one row and the `len == 1` branch is taken almost every time. This is my estimate from the generator and I have not measured it. The new code only differs from `main` when `len > 1`. The new unit test covers the batch boundary and output-batch cuts, but only for `Inner` and one hand-built shape. Could we add a low-cardinality case to the existing fuzz harness that compares sort-merge join with hash join? For example join on `a` only, or draw keys from `0..5`. Using the existing `batch_sizes` list (`[1, 2, 7, 49, 50, 51, 100]`), for `Inner`, `Left` and `Full`, filtered and unfiltered, would exercise runs that end at a buffered batch boundary and runs capped by the output batch. Extending `JoinFuzzTestCase` should be enough, with no new harness. The fuzz tests are also gated behind `extended_tests`, so the PR tier does not run them. A low-cardinality case would be worth having for any later change to this loop. -- 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]
