Skip to content

fix: consider input sizes before reusing join partitioning - #25787

Open
ShayanGho wants to merge 4 commits into
apache:mainfrom
ShayanGho:issue-25302-reference-size
Open

ShayanGho wants to merge 4 commits into
apache:mainfrom
ShayanGho:issue-25302-reference-size

Conversation

@ShayanGho

@ShayanGho ShayanGho commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #25302.

Rationale for this change

A small range-partitioned input can become the reference for a much larger join input, forcing the larger input to adopt the smaller input's partition boundaries.

What changes are included in this PR?

Reject a selected reference when another input in the co-partitioned group has a strictly larger PlanSize. This allows the existing hash-partitioning fallback to apply.

Equal or incomparable sizes do not trigger the new rejection.

What is the testing strategy for this PR?

Added two regression tests:

  • A smaller reference triggers hash fallback. This test failed before the fix.
  • Equal-sized inputs preserve the reference's range partitioning.

Validation:

  • Distribution optimizer tests.
  • Extended workspace tests with the required features.
  • cargo fmt --all.
  • cargo clippy --all-targets --all-features -- -D warnings.

Are there any user-facing changes?

Join plans can use hash repartitioning instead of adapting a larger input to a smaller input's existing partitioning.

@github-actions github-actions Bot added optimizer Optimizer rules core Core DataFusion crate labels Sep 27, 2026
@codecov-commenter

codecov-commenter commented Sep 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.80%. Comparing base (6047791) to head (1aab317).
⚠️ Report is 95 commits behind head on main.

Files with missing lines Patch % Lines
...er/src/ensure_requirements/enforce_distribution.rs 92.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25787      +/-   ##
==========================================
+ Coverage   82.64%   82.80%   +0.15%     
==========================================
  Files        1147     1149       +2     
  Lines      445883   452257    +6374     
  Branches   445883   452257    +6374     
==========================================
+ Hits       368513   374499    +5986     
+ Misses      54982    54958      -24     
- Partials    22388    22800     +412     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ShayanGho

Copy link
Copy Markdown
Contributor Author

Hello @jayzhan211 @gene-bordegaray, would either of you have time for this one too? It's a 13-line change in enforce_distribution_relationships for #25302: don't adopt a reference child's partitioning when another input is strictly larger. Merged with main, and it passes alongside the new native-reference test from apache/datafusion#24766.

@jayzhan211 jayzhan211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ShayanGho , 1 suggestion

}
};

// Reject a reference when another child in the co-partitioned group

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this fires, the larger child is shuffled in both plans (it is unsatisfied), so rejecting the reference never saves a shuffle; it only adds a shuffle of the reference. With strict >, that also happens when the sizes are close: at 10M vs 11M rows, 21M rows get shuffled instead of 11M. It also reverses #24766's native-reference rule, and range_fallback_preserves_target_parallelism now asserts the opposite of what it was added to guard. Could we reject only when the reference is small, reusing JoinSelection's CollectLeft thresholds (hash_join_single_partition_threshold{,_rows})? The issue's 100-row case would still fall back to hash, inputs of similar size would keep a single shuffle, and #24766's test could keep its original assertion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, done. The reference is now rejected only when it is below the CollectLeft thresholds and another input is above them. Inputs on the same side of the thresholds keep the reference, so only one side is shuffled (range_hash_join_preserves_large_reference covers the 10M vs 11M case), and range_fallback_preserves_target_parallelism is back to its original assertion.

Reject a reference child only when it is below the CollectLeft
thresholds and another co-partitioned input is above them, so inputs of
comparable size keep the reference and only one side is shuffled.
Restores the original assertion of range_fallback_preserves_target_parallelism.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

enforce_distribution_relationships should consider size of unsatisfied children before selecting a reference child in asymmetric joins

3 participants