Skip to content

fix: preserve fetch across distribution reoptimization - #24809

Open
xudong963 wants to merge 1 commit into
apache:mainfrom
massive-com:fix/ensure-requirements-preserve-fetch
Open

fix: preserve fetch across distribution reoptimization#24809
xudong963 wants to merge 1 commit into
apache:mainfrom
massive-com:fix/ensure-requirements-preserve-fetch

Conversation

@xudong963

Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

Reoptimizing an already optimized physical plan could silently remove a pushed-down LIMIT stored on SortPreservingMergeExec or CoalescePartitionsExec. This could make a query return more rows than requested.

What changes are included in this PR?

  • Consume a removed fetch when a replacement merge operator is inserted.
  • Remember the outermost removed fetch-capable distribution operator and rebuild it around the optimized child when no replacement consumes its limit.
  • Preserve the minimum effective fetch across nested distribution operators.
  • Carry a fetched ordered merge's limit to a replacement sort when order-preserving variants are removed.

What is the testing strategy for this PR?

Added targeted physical optimizer regression tests covering:

  • reoptimizing a fetched SortPreservingMergeExec;
  • reoptimizing a fetched CoalescePartitionsExec;
  • moving a fetched ordered merge's limit to a replacement sort;
  • updating an existing fetched single-partition merge snapshot that previously
    encoded the incorrect removal of its limit.

Validated with:

# Fails on e4cf35cbc (current main before this patch): both regression tests fail
cargo test -p datafusion --test core_integration physical_optimizer::enforce_distribution::preserve_fetch_when_reoptimizing

# Passes with this patch
cargo test -p datafusion --test core_integration physical_optimizer::enforce_distribution::preserve_fetch_when_reoptimizing
cargo test -p datafusion --test core_integration physical_optimizer::enforce_distribution::move_fetch_to_replacement_sort
cargo test -p datafusion --test core_integration physical_optimizer::enforce_distribution::test_replace_order_preserving_variants_with_fetch
cargo fmt --all -- --check
cargo clippy --all-targets --all-features -- -D warnings
RUST_BACKTRACE=1 cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli --workspace --lib --tests --bins --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption

Are there any user-facing changes?

Queries preserve their requested global limit when physical distribution requirements are optimized more than once. There are no changes to SQL behavior other than fixing the incorrect result, and no changes to documented public APIs.

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

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.35802% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.52%. Comparing base (e4cf35c) to head (1d957a5).

Files with missing lines Patch % Lines
...er/src/ensure_requirements/enforce_distribution.rs 91.35% 4 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24809   +/-   ##
=======================================
  Coverage   81.52%   81.52%           
=======================================
  Files        1123     1123           
  Lines      406148   406191   +43     
  Branches   406148   406191   +43     
=======================================
+ Hits       331124   331166   +42     
+ Misses      55659    55658    -1     
- Partials    19365    19367    +2     

☔ 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.

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.

EnsureRequirements can silently drop fetch during distribution reoptimization

2 participants