andygrove opened a new issue, #2081:
URL: https://github.com/apache/datafusion-ballista/issues/2081

   **Describe the bug**
   
   The adaptive (AQE) planner decides whether to broadcast a join's build side 
in `DynamicJoinSelectionExec::supports_collect_by_thresholds`:
   
   ```rust
   let Ok(stats) = plan.partition_statistics(None) else { return false; };
   
   if let Some(byte_size) = stats.total_byte_size.get_value() {
       *byte_size != 0 && *byte_size < threshold_byte_size      // 10 MB
   } else if let Some(num_rows) = stats.num_rows.get_value() {
       *num_rows != 0 && *num_rows < threshold_num_rows         // 1,000,000
   } else {
       false
   }
   ```
   
   When `total_byte_size` is known this is a size check, which is what we want. 
But when it is `Absent` the decision falls back to a **row count** compared 
against `hash_join_single_partition_threshold_rows` (default 1,000,000). A row 
count says nothing about size, so a build side of up to 1M arbitrarily wide 
rows can be collected and replicated to every probe task while the 10 MB guard 
never gets a chance to apply.
   
   This is not a rare corner. `total_byte_size` is `Absent` for exactly the 
intermediates whose size we cannot guess:
   
   1. **Join outputs, always.** DataFusion's `estimate_join_statistics` 
hardcodes `total_byte_size: Precision::Absent` and keeps only an estimated row 
count, so every join result has unknown size.
   2. **Anything with a variable-width column.** 
`Statistics::calculate_total_byte_size` sums `DataType::primitive_width()` 
across the schema; that returns `None` for 
`Utf8`/`LargeUtf8`/`Utf8View`/`Binary` (and `Boolean`), so a single string 
column makes it bail out to `total_byte_size.to_inexact()`. `to_inexact()` on 
`Absent` is still `Absent`, so once the size is lost a projection cannot 
recover it.
   
   In TPC-H that covers most dimension-side join results, since they carry 
names, addresses, or comments.
   
   **To Reproduce**
   
   TPC-H SF1000, 2 executors x (8 cores / 56 GiB), `target_partitions=32`, 
`prefer_hash_join=false`, adaptive planner on, with 
`RUST_LOG=info,ballista_scheduler::state::aqe=debug`. Q2 alone produces:
   
   ```
   to_actual_join - plan_id: 1, decision: CollectLeft left: (Inexact(799140)   
| Absent), right: (Exact(10000000) | Absent)
   to_actual_join - plan_id: 7, decision: CollectLeft left: (Inexact(639312)   
| Absent), right: (Inexact(159969600) | Inexact(3839270400))
   to_actual_join - plan_id: 5, decision: CollectLeft left: (Inexact(10000000) 
| Inexact(320000000)), right: (Exact(25) | Exact(400))
   to_actual_join - plan_id: 2, decision: CollectLeft left: (Inexact(3196560)  
| Absent), right: (Exact(25) | Absent)
   ```
   
   `plan_id: 1` and `plan_id: 7` are broadcast on the row rule alone (799,140 
and 639,312 rows, size unknown). These are `part`/`supplier`-derived rows 
carrying string columns, so the real payload replicated to all 32 tasks is 
plausibly hundreds of MB rather than the 10 MB the byte threshold is meant to 
cap.
   
   The same run also shows how easily the size is lost. Two decisions over the 
same 25-row `nation` table:
   
   - `plan_id: 5`, right side `Exact(25) | Exact(400)` — an all-numeric 
projection, so bytes are recomputed as rows x width.
   - `plan_id: 2`, right side `Exact(25) | Absent` — the schema carries a 
string column, so the recompute gives up.
   
   **Expected behavior**
   
   The broadcast decision should be driven by an estimate of the build side's 
**size**. When `total_byte_size` is unavailable, Ballista should estimate it 
(rows x estimated row width, from the schema's fixed-width columns plus 
per-column statistics or a configurable default for variable-width columns) and 
test that against the byte threshold, rather than treating "fewer than N rows" 
as a proxy for "small".
   
   For reference, Spark drives broadcast selection from 
`spark.sql.autoBroadcastJoinThreshold` in bytes, and estimates a per-row size 
from the schema (`EstimationUtils.getSizePerRow`) when it must; it has no 
row-count threshold standing in for size.
   
   **Additional context**
   
   Found while benchmarking TPC-H SF1000 under both planners (2x8 cores, 
node-local data, 1 iteration). AQE-on and AQE-off totalled 6320.7 s and 6378.2 
s respectively — a wash overall, but the per-query results move in both 
directions, and the queries where AQE loses most (Q5 700.5 vs 542.7, Q8 801.2 
vs 657.1, Q9 1025.2 vs 891.8, Q18 820.9 vs 620.7) are join-heavy ones where 
this fallback is most likely to be making the call. Whether over-eager 
broadcasts explain those regressions is not yet established — this issue is 
about the decision rule being size-blind, which is true regardless.
   
   There are related gaps that are worth tracking separately and are not 
proposed here:
   
   - `stats_for_partitions` 
(`ballista/core/src/execution_plans/shuffle_reader.rs`) reports shuffle 
`num_rows`/`total_byte_size` as `Exact` from measured output, but sets 
`column_statistics` to all-unknown (there is a `TODO stats: add column 
statistics to PartitionStats`), and the wire `ColumnStats` message has no 
`byte_size` field. So Ballista measures real sizes at every shuffle boundary 
and then cannot carry them into downstream planning.
   - The two DataFusion behaviours above (joins discarding `total_byte_size`; 
`calculate_total_byte_size` ignoring the existing `ColumnStatistics::byte_size` 
field for variable-width columns) would be better fixed upstream.
   


-- 
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]

Reply via email to