andygrove commented on PR #2434:
URL:
https://github.com/apache/datafusion-ballista/pull/2434#issuecomment-5647182869
Thanks both. @milenkovicm for the approval, and @comphead for the second
pass — I took essentially all of it. Net **−232 lines** (386 insertions, 618
deletions) across the branch, and **410 passed / 0 failed**, clippy and rustfmt
clean.
The load-bearing check: the only snapshots insta accepted were the four in
`stage_build_side.rs`, and their plan shapes and `plan_id`s are byte-identical
to before the refactor. Nothing in `test/join_selection.rs` or the
plan-stability files moved, which is the evidence that this round is
behaviour-preserving.
I verified your three structural claims rather than taking them on faith,
and all three hold: `create_partitioning(self, ..)` does consume (so the clone
in `join_key_partitioning` was genuinely dead), `partition_mode` is
`Partitioned` only when `!null_aware`, and the local `selection_state` can only
be `Unknown` when `self.selection_state` already is — so
`with_selection_state(Unknown)` really was copying eleven fields to change
nothing.
### Taken
**Production.** Dropped the `join` payload (the arm rebuilds from the
visited node); `build_side: JoinSide` → `build_is_right: bool`, which deletes
the unreachable `JoinSide::None` arm, its error string and the `JoinSide`
import from both files; collapsed the two `(Unknown, Partitioned)` arms into
one with the join-type condition moved inside `requires_build_staging` (which
retires the "caller is responsible for the fourth condition" paragraph);
removed `with_selection_state` and restored `to_partitioned`'s inline body;
`BuildStagingLimits` → plain params; `probe_stats` hoisted beside
`build_stats`; `join_key_partitioning` returns a tuple by value over the owned
`Vec`; `exchange_on` → `SelectJoinRule::ensure_exchange`.
On the naming: agreed `exchange_on` read as "on the join keys". The receiver
keeps the `fetch_add` laziness without the generic, as you suggested. I also
took the doc line about `inputs_already_partitioned` — the structural
comparison is deliberate, since the only exchange worth reusing is one this
rule just placed, and that is now written down so nobody unifies them later.
**Tests.** Six single-assert staging tests folded into two behind a
`stages(build, probe, threshold)` predicate; the two hand-partitioned join-type
lists merged into one loop over all ten with `collect_left_broadcast_safe` as
the oracle (your point that a new DataFusion join type fails neither current
list is exactly right, and the merged form does catch it); `q8_shaped_action*`
un-forked from `run_to_actual_join`, which gained a `stage_build_side` param;
the fourth `StatisticsExec` builder parameterised with the other three as
wrappers; `mock_partitions_with_statistics_no_data` delegating to
`mock_partitions_with_size`; `estimated_statistics` using
`Statistics::to_inexact`.
You were right that the fixtures in `stage_build_side.rs` were a straight
copy — `ballista_ctx`, `narrow_schema`, `register_stats_table` and `MB` now
live in `test/mod.rs` and both files import them, so "the shipped session
config" means one thing.
Dropped `does_not_stage_a_left_join` and
`exchange_on_wraps_a_non_exchange_child`. On the first, your read was better
than mine: `Left` is already a row of the merged loop with the same q8-shaped
statistics, so the only unique content was a plan snapshot that needs
re-blessing on every unrelated `plan_id` shift.
Also fixed the shadowed `use datafusion::common::{ColumnStatistics,
Statistics, stats::Precision};` — good catch, since as you say `-D warnings`
never would have.
### Not taken
**The exchange-guard depth**, deliberately. The fire-at-most-once bound is
unaffected, because staging always produces a *direct* `ExchangeExec` child,
which the guard does catch. In the `FilterExec(ExchangeExec(..))` shape the
staged measurement is of the filter's output rather than the inner shuffle's,
so it is an extra boundary but not redundant work, and a subtree-wide search
would decline legitimately useful cases — a build side that is itself a join
over an already-resolved broadcast exchange, for one. Noted as a follow-up;
happy to tighten if you think the shape is likelier than I am reading it.
**Threading one `StatisticsContext` through `to_actual_join`.** Agreed it is
the natural moment, and agreed this PR adds no new statistics computation. But
it is a change to a path five callers share, with no test that would catch a
memo-sharing mistake, so I would rather it not ride along behind a
behaviour-preserving cleanup. Separate PR.
### @milenkovicm
Law of Demeter: taken, and it lines up with comphead's item —
`requires_build_staging` now takes `min_probe_ratio` and
`max_estimate_multiple` as plain params rather than reaching through
`&BallistaConfig`, and `broadcast_threshold_bytes` is passed the same way, so
the mix-and-match is gone.
Config wording: also taken. All three `stage_build_side*` entries now say
explicitly that staging applies only where the build side's size is an estimate
rather than a measurement — an exact size over the budget is a fact no further
measurement can change. Docs regenerated.
And yes on #1916 and cardinality estimation as the follow-up. Better
cardinalities would shrink the window this heuristic exists to cover, which is
the right direction.
### Benchmarks and docs
I reverted the `benchmarking.md` change entirely, per your point. It pinned
a row to an unmerged PR commit behind an unowned TODO, and it was the only
`{note}` admonition in `docs/source`. The deeper problem is the one you found
in Q1 vs Q6: both are join-free, one improved 4% and the other regressed 7%, so
a ±7% noise floor was being published as measured signal next to a claim that
this PR accounts for almost all of the improvement.
Those numbers also predate the join-type guard, so they are stale
regardless. I will re-measure on the cluster and land the docs separately after
merge, with the iteration count and the regression band stated rather than
rounded away.
### CI
The S3 failures look like the MinIO issue you linked (#2446) rather than
anything here.
--
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]
