viirya commented on code in PR #24536:
URL: https://github.com/apache/datafusion/pull/24536#discussion_r3839025089
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -470,12 +470,42 @@ pub(crate) fn estimate_join_statistics(
join_type: &JoinType,
schema: &Schema,
) -> Result<Statistics> {
+ // Width of one output row, from the sides this join emits. Without it a
join reports
+ // no size and `hash_join_single_partition_threshold` falls back to
counting rows.
+ let width = |stats: &Statistics| match (
Review Comment:
Why an f64 average row width rather than staying in `Precision`?
`stats_cartesian_product` in `cross_join.rs` derives its output size with
`Precision` arithmetic throughout
(`left.total_byte_size.multiply(&right_row_count).add(...)`), which keeps
integer precision and gets the `Exact`/`Inexact` lattice for free. Here the
width round-trips through f64. Was `Precision` arithmetic considered and
rejected — presumably because the per-side row-count multiplication that works
for a cartesian product doesn't map onto an equi-join? If f64 is the right
call, a brief comment saying why would help, since the neighbouring code sets a
different precedent.
Also: reading through `get_value()` means the result is `Inexact` even when
both sides are `Exact`. I assume that's deliberate (an average row width is
inexact regardless), but worth confirming it's intent rather than an artifact.
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -470,12 +470,42 @@ pub(crate) fn estimate_join_statistics(
join_type: &JoinType,
schema: &Schema,
) -> Result<Statistics> {
+ // Width of one output row, from the sides this join emits. Without it a
join reports
+ // no size and `hash_join_single_partition_threshold` falls back to
counting rows.
+ let width = |stats: &Statistics| match (
+ stats.total_byte_size.get_value(),
+ stats.num_rows.get_value(),
+ ) {
+ (Some(bytes), Some(rows)) if *rows > 0 => Some(*bytes as f64 / *rows
as f64),
+ _ => None,
+ };
+ // The boolean a mark join appends is one bit per row.
+ const MARK_COLUMN_WIDTH: f64 = 1.0 / 8.0;
+ let output_width = match join_type {
+ JoinType::LeftSemi | JoinType::LeftAnti => width(&left_stats),
+ JoinType::RightSemi | JoinType::RightAnti => width(&right_stats),
+ JoinType::Inner | JoinType::Left | JoinType::Right | JoinType::Full =>
{
+ width(&left_stats)
+ .zip(width(&right_stats))
+ .map(|(left, right)| left + right)
+ }
+ JoinType::LeftMark => width(&left_stats).map(|w| w +
MARK_COLUMN_WIDTH),
+ JoinType::RightMark => width(&right_stats).map(|w| w +
MARK_COLUMN_WIDTH),
+ };
+
let join_stats =
estimate_join_cardinality(join_type, left_stats, right_stats, on,
null_equality);
let (num_rows, total_byte_size, column_statistics) = match join_stats {
Some(stats) => (
Precision::Inexact(stats.num_rows),
- stats.total_byte_size,
+ match (stats.total_byte_size, output_width) {
+ // Only fill a gap: a size the join derived from its column
statistics
+ // knows which columns it keeps, which an average row width
cannot.
+ (Precision::Absent, Some(width)) => {
+ Precision::Inexact((stats.num_rows as f64 * width) as
usize)
Review Comment:
Since `total_byte_size` feeds `hash_join_single_partition_threshold` in
`join_selection.rs`, this estimate now decides whether a join becomes
`CollectLeft` — which is exactly the effect your benchmarks show. For
fixed-width columns the average row width is the true width, but for
`Utf8`/`List` it can sit well below the widest rows. Have you considered
whether an under-estimate here can flip a large build side into `CollectLeft`
and raise memory use? If that's a real risk, rounding the derived size up — or
only filling the gap for fixed-width schemas — might be worth considering.
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -470,12 +470,42 @@ pub(crate) fn estimate_join_statistics(
join_type: &JoinType,
schema: &Schema,
) -> Result<Statistics> {
+ // Width of one output row, from the sides this join emits. Without it a
join reports
+ // no size and `hash_join_single_partition_threshold` falls back to
counting rows.
+ let width = |stats: &Statistics| match (
+ stats.total_byte_size.get_value(),
+ stats.num_rows.get_value(),
+ ) {
+ (Some(bytes), Some(rows)) if *rows > 0 => Some(*bytes as f64 / *rows
as f64),
+ _ => None,
+ };
+ // The boolean a mark join appends is one bit per row.
+ const MARK_COLUMN_WIDTH: f64 = 1.0 / 8.0;
+ let output_width = match join_type {
+ JoinType::LeftSemi | JoinType::LeftAnti => width(&left_stats),
Review Comment:
Is this arm reachable in practice? It only applies when the cardinality
estimate returned `Absent` for `total_byte_size` while the left input still has
both `total_byte_size` and `num_rows` — and your semi-join test takes the
column-derived path instead, so it doesn't exercise this branch. If the
combination does occur (a source with a file-level size but no per-column
`byte_size`, say), a test with left stats where `total_byte_size` is present
but per-column `byte_size` is absent would pin the behaviour down.
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -470,12 +470,42 @@ pub(crate) fn estimate_join_statistics(
join_type: &JoinType,
schema: &Schema,
) -> Result<Statistics> {
+ // Width of one output row, from the sides this join emits. Without it a
join reports
+ // no size and `hash_join_single_partition_threshold` falls back to
counting rows.
+ let width = |stats: &Statistics| match (
+ stats.total_byte_size.get_value(),
+ stats.num_rows.get_value(),
+ ) {
+ (Some(bytes), Some(rows)) if *rows > 0 => Some(*bytes as f64 / *rows
as f64),
+ _ => None,
+ };
+ // The boolean a mark join appends is one bit per row.
+ const MARK_COLUMN_WIDTH: f64 = 1.0 / 8.0;
Review Comment:
`1/8` byte matches `Field::new("mark", DataType::Boolean, false)` —
non-nullable, so there's no validity buffer to account for. Noting I checked,
in case that field ever becomes nullable and this constant needs revisiting.
--
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]
