comphead commented on code in PR #2434:
URL:
https://github.com/apache/datafusion-ballista/pull/2434#discussion_r3993787485
##########
ballista/scheduler/src/state/aqe/execution_plan/dynamic_join.rs:
##########
@@ -606,6 +664,82 @@ impl DynamicJoinSelectionExec {
/// join always uses hash join); an unknown build size
(`build_max_partition_bytes
/// == None`) is treated as fitting too, since there is no evidence to force a
/// fallback.
+/// How much larger the probe side must be before staging the build side alone
+/// is worth an extra round trip.
+///
+/// Staging serialises the two shuffles that `Repartition` would otherwise run
+/// concurrently, so the deferral costs at most the *smaller* side's runtime.
+/// Requiring an order of magnitude keeps that cost well under the probe-side
+/// shuffle it stands to avoid entirely.
+const STAGE_BUILD_SIDE_MIN_PROBE_RATIO: usize = 10;
+
+/// How far over the broadcast budget an *estimated* build side may sit and
+/// still be worth measuring.
+///
+/// TPC-H q8's filtered `part` scan estimates roughly 10x over budget, because
+/// the planner falls back to `default_filter_selectivity` for
+/// `p_type = '...'`, and measures three orders of magnitude under it. A raw
+/// fact-table scan sits far beyond this multiple and is shuffled without the
+/// extra round trip.
+const STAGE_BUILD_SIDE_MAX_ESTIMATE_MULTIPLE: usize = 32;
+
+/// Whether to shuffle only the prospective build side now and re-decide the
+/// join once its measured size is known, rather than shuffling both sides.
+///
+/// [`JoinSelectionAction::Repartition`] shuffles both inputs at once, which
for
+/// a fact-table probe side commits to the single most expensive stage in the
+/// query *before* any measurement exists. When the build side's size is only a
+/// guess, that guess is the sole reason the join is not a broadcast, and the
+/// probe side dwarfs it, one cheap stage buys an exact number and often flips
+/// the join to `CollectLeft`, leaving the probe side never shuffled at all.
+///
+/// Each of the three conditions is necessary:
+///
+/// * the build estimate must be **inexact**. An exact size over budget is a
+/// fact rather than a guess, so measuring it again cannot change the outcome
+/// and would only serialise two shuffles that could run concurrently.
+/// * the estimate must be **plausibly wrong enough to flip**. Past
+/// [`STAGE_BUILD_SIDE_MAX_ESTIMATE_MULTIPLE`] the side is large on any
+/// reading, and no measurement brings it under budget.
+/// * the probe side must be **much larger**, per
+/// [`STAGE_BUILD_SIDE_MIN_PROBE_RATIO`], which is what bounds the cost of
+/// being wrong.
+///
+/// Both sides are compared in bytes. A missing probe-side size declines: with
+/// nothing to compare against there is no evidence the round trip pays off.
+fn should_stage_build_side(
Review Comment:
Would prefer something like
`requires_build_staging`, `build_staging_is_neeed`, etc
--
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]
