andygrove opened a new pull request, #2444:
URL: https://github.com/apache/datafusion-ballista/pull/2444
# Which issue does this PR close?
Closes #.
No issue filed yet. Split out of #2434 at review request, so that the two
behaviour changes there can land and be reverted independently.
# Rationale for this change
`supports_collect_by_thresholds` applied the row threshold *before* the
schema-derived byte estimate and rejected on either, so a build side
comfortably inside the byte budget could still be vetoed on row count alone. A
1M-row `i64` key column is 8 MB against a 128 MiB default budget, 16x under.
The config already documents the row threshold as a fallback rather than a
ceiling:
> Row-count threshold below which a hash join's smaller side is promoted to
CollectLeft and lowered via the broadcast pattern, **used as a fallback when
byte-size statistics are unavailable.**
and #2084's description says the same ("keeping DataFusion's row threshold
as the absent-stats fallback"). The code and its tests applied it as an
additional ceiling instead. This makes the code match that stated intent: when
a byte estimate can be formed it decides, and the row rule applies only when it
cannot.
Byte statistics being absent is the common case rather than the exception —
DataFusion discards `total_byte_size` on every join, and
`Statistics::calculate_total_byte_size` only rebuilds it when every column has
a fixed width, so a single `Utf8` column loses it for good. That is why the
estimate matters more than the row count here.
To be clear about scope, this is a deliberate behaviour change, not a bug
fix in the sense of restoring prior behaviour.
`row_threshold_remains_a_ceiling` asserted the old semantics on purpose and is
replaced here.
# What changes are included in this PR?
`supports_collect_by_thresholds` now:
- returns early when `threshold_num_rows == 0`, so `0` still disables the
whole row-count path as documented;
- forms the schema-derived byte estimate and holds it to the byte threshold
alone;
- consults the row threshold only when no estimate can be formed, which
happens only when the row count is large enough to overflow the width
multiplication — a side far past any budget, which the row rule then rejects.
A known `total_byte_size` still short-circuits both paths and remains
authoritative.
**Tests.** `row_threshold_remains_a_ceiling` is replaced by four tests
covering each branch: a row count at the threshold over narrow rows now
collects; the same row count over wide rows is still rejected by the estimate;
the row rule decides when the estimate overflows; and `0` disables the
row-count path. `broadcast_threshold_rows_drives_fallback_decision` is reworked
to assert the new semantics, including the `0` case.
`cargo test -p ballista-scheduler --lib`: 412 passed, 0 failed. Clippy clean.
# Are there any user-facing changes?
No API changes.
One behaviour change: `ballista.optimizer.broadcast_join_threshold_rows` no
longer vetoes a build side whose byte estimate is inside
`broadcast_join_threshold_bytes`. Anyone relying on it as a hard row cap will
see more joins broadcast. Setting it to `0` still disables the row-count path
entirely, as documented.
No config docs change: the existing entry already describes the row
threshold as a fallback, which is what the code now does.
--
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]
