andygrove opened a new pull request, #2066:
URL: https://github.com/apache/datafusion-ballista/pull/2066
# Which issue does this PR close?
Closes #2065.
Also resolves the q4 / q78 half of #2046 (the set-operation half, q38 / q87,
is
a separate DataFusion-side bug handled in #2064).
# Rationale for this change
Lowering a plan into stages rewrites nodes, and a rewrite can drop an
ordering
its parent depends on. Ballista's broadcast promotion turns a
`SortMergeJoinExec` into a `HashJoinExec(CollectLeft)`, which does not
preserve
input order — but nothing re-establishes that order, so a parent
`SortMergeJoinExec` ends up merging unsorted input.
A merge join given unsorted input does not error. It stops matching once the
merge cursors diverge, so rows are **silently dropped** and the query
returns a
wrong answer.
In the q78 plan the outer `SortMergeJoinExec` had a `SortExec` on its right
input and nothing on its left, because the inner SMJ that used to supply that
ordering had been replaced by a broadcast hash join:
```
SortMergeJoinExec: join_type=Left, on=[(ss_sold_year, cs_sold_year), ...]
...
HashJoinExec: mode=CollectLeft, join_type=Right, ... <-- left
input: no ordering
UnresolvedShuffleExec: stage=8, broadcast=true, upstream_partitions:
16
...
SortExec: expr=[cs_item_sk ASC, cs_customer_sk ASC], ... <-- right
input: sorted
```
Disabling the promotion isolated the cause —
`broadcast_join_threshold_bytes=0`
or `broadcast_sort_merge_join_enabled=false` both restore the correct answer.
This is distinct from #1055, which concerns which *join types* are safe to
broadcast (already guarded by `collect_left_broadcast_safe`). Here the join
type
is fine; the ordering contract is what breaks.
# What changes are included in this PR?
- `restore_required_input_ordering`: after lowering rewrites a node's
children,
re-establish any `Hard` input ordering a child no longer carries. Sorts are
only added, never removed, so an input that already satisfies its
requirement
is untouched.
- `validate_sort_merge_join_orderings`: verify every `SortMergeJoinExec` in
the
planned stages has inputs satisfying the join's required ordering, so this
failure mode surfaces as an error instead of wrong results.
- Un-skip TPC-DS q4 and q78 in the correctness gate (86 → 88 queries).
`Soft` requirements are deliberately ignored — they are opportunistic and an
operator works without them.
## Verification
SF1, 1 scheduler / 1 executor, `target_partitions=16`:
| | Before | After |
|---|---|---|
| q78 | 73 rows vs 100 | verified OK (100 rows) |
| q4 | 0 rows vs 8 | verified OK (8 rows) |
| minimal repro (q78 CTEs, no ORDER BY/LIMIT) | 73 rows vs 223 | 223 rows |
Full SF1 TPC-DS gate: **88/88 verified against single-process DataFusion**,
under both `prefer_hash_join=false` and `prefer_hash_join=true`. The new
validation reports **0 false positives** across all 88 queries under both
settings, which is why it is a hard error rather than a debug assertion.
`cargo test --workspace`, `cargo clippy --all-targets --workspace -- -D
warnings`, and `cargo fmt --all` are clean.
## Note on the q78 framing
q78 was originally reported under #2046 as "distributed LIMIT drops rows".
That
framing is a red herring: the wrong answer does not depend on `ORDER BY` or
`LIMIT` and reproduces with neither. Wrapping the query in `count(*)` changes
the plan enough to hide the bug, which is what made it look limit-related.
# Are there any user-facing changes?
Yes — queries where a broadcast-promoted join feeds a sort-merge join now
return correct results instead of silently dropping rows. A plan that would
hit
this now fails with an explicit error rather than producing a wrong answer.
No
API changes.
--
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]
