andygrove opened a new pull request, #2130: URL: https://github.com/apache/datafusion-ballista/pull/2130
# Which issue does this PR close? Backport of the correctness fix in #2072 to `branch-54`. Partially addresses #2046. # Rationale for this change `54.0.0` added a static-planner rewrite that converts a `SortMergeJoinExec` with a small build side into a broadcast `CollectLeft` hash join, gated by `ballista.optimizer.broadcast_sort_merge_join_enabled`. The rewrite produces incorrect results, and the key defaults to `true`, so the bad path is on out of the box on `54.0.0` and `54.1.0`. This is not an edge-case path. Ballista sets `datafusion.optimizer.prefer_hash_join = false` by default (#1648), so joins are planned as `SortMergeJoinExec` unless a user opts out, and the conversion then fires on any join with a side under `broadcast_join_threshold_bytes` (default 10 MB). The conversion also worked against the reason sort-merge is the default. DataFusion's hash join has no spill support, so a `CollectLeft` build side must fit in memory in every parallel task, while sort-merge spills. On `main`, #2072 removed the rewrite and its config key outright. Deleting a public config key is a breaking change, which is not appropriate for the 54 patch line, so this PR takes the non-breaking half of that fix: flip the default to `false` and keep the key so anyone depending on the old plan shape can opt back in. Removal stays on `main` for `55.0.0`. # What changes are included in this PR? - Default `ballista.optimizer.broadcast_sort_merge_join_enabled` to `false`. - Update the config docs and the `broadcast_sort_merge_join_enabled_by_default` unit test to pin the new default (renamed to `..._disabled_by_default`). - Note the corrected default in the `54.0.0` upgrade guide, with a warning for users already on `54.0.0`/`54.1.0`. - Regenerate the 11 affected TPC-H plan-stability goldens (q2, q5, q7, q8, q9, q10, q11, q15, q16, q20, q21). The planner code and the `distributed_broadcast_sort_merge_join_plan` test are untouched: that test opts in explicitly, so it still covers the conversion for anyone who enables it. # Are there any user-facing changes? Yes, and that is the point of the fix. Joins that were silently converted to broadcast hash joins now stay as sort-merge joins, so query plans change and incorrect results go away. No public API is removed or renamed, and no proto/wire format changes, so this is not a breaking change. Verification: the regenerated goldens are **byte-identical** to the ones #2072 produced on `main`, confirming this default flip yields exactly the same plans as the upstream removal. `cargo fmt --check`, clippy on `ballista-core`/`ballista-scheduler` with `--all-targets --all-features`, and the full `ballista-core` + `ballista-scheduler` test suites are clean. -- 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]
