andygrove opened a new pull request, #2072: URL: https://github.com/apache/datafusion-ballista/pull/2072
# Which issue does this PR close? Partially addresses #2046 (q4 and q78 only; q38 and q87 are unrelated INTERSECT/EXCEPT divergences and stay skipped). Closes #. # Rationale for this change `54.0.0` added a static-planner rewrite that converted a `SortMergeJoinExec` with a small build side into a broadcast `CollectLeft` hash join, gated by `ballista.optimizer.broadcast_sort_merge_join_enabled` (default `true`). The rewrite produces incorrect results, so the only safe value for the key is `false`. A config knob whose correct setting is always "off" is better removed along with the code it gates. This also corrects an assumption recorded in #2046, which reasoned that the q4/q78 divergences were distinct from the broadcast correctness issue in #1055 because they reproduce with `prefer_hash_join=false` — "i.e. sort-merge joins, no broadcast". That inference does not hold while this conversion is enabled by default: with it on, `prefer_hash_join=false` still yields a broadcast join, so those runs were exercising the broadcast path after all. # What changes are included in this PR? - Remove `convert_sort_merge_to_hash_join` and its `strip_sort` helper from `DefaultDistributedPlanner`. A `SortMergeJoinExec` is no longer a broadcast candidate; only a `HashJoinExec(Partitioned)` is. - Remove the `ballista.optimizer.broadcast_sort_merge_join_enabled` config key and its `BallistaConfig` accessor. - Drop the test asserting the conversion happens, and the test asserting the threshold gated it (now vacuous). `distributed_sort_merge_join_unchanged_when_flag_disabled` becomes `distributed_sort_merge_join_is_never_broadcast`, since that is now unconditional behavior. - Re-enable TPC-DS q4 and q78 in the correctness gate. - Add a `55.0.0` upgrade guide; drop the stale "not been released yet" note from the `54.0.0` guide. Broadcast promotion of a `HashJoinExec` under `ballista.optimizer.broadcast_join_threshold_bytes` is unchanged, as is the correctness guard that demotes a DataFusion-promoted `CollectLeft` join with an unsafe join type. # Are there any user-facing changes? Yes, two, both documented in `docs/source/upgrading/55.0.0.md`: 1. `ballista.optimizer.broadcast_sort_merge_join_enabled` no longer exists. Ballista rejects unknown configuration keys, so a session that still sets it now fails with ``configuration key `optimizer.broadcast_sort_merge_join_enabled` does not exist``. This is a breaking config change against `54.0.0`. 2. A `SortMergeJoinExec` is never converted to a broadcast join, so plans that previously broadcast under `prefer_hash_join=false` now execute as sort-merge joins over repartitioned inputs. Since `datafusion.optimizer.prefer_hash_join` defaults to `true`, most workloads are unaffected. ## Verification Locally: `cargo clippy --all-targets --workspace --all-features -- -D warnings` passes, and the `ballista-scheduler` planner, `ballista-core` config, and `tpcds` bin tests pass. The q4/q78 fix itself is **not** verified locally — it needs SF1 TPC-DS data and a live cluster. The TPC-DS SF1 workflow runs on this PR and is the real check; if q4 or q78 still diverge, those skip entries should go back with an updated reason. -- 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]
