Skip to content

Cap SortPreservingMerge statistics by fetch#23359

Open
discord9 wants to merge 1 commit into
apache:mainfrom
discord9:fix/spm-fetch-statistics-upstream
Open

Cap SortPreservingMerge statistics by fetch#23359
discord9 wants to merge 1 commit into
apache:mainfrom
discord9:fix/spm-fetch-statistics-upstream

Conversation

@discord9

@discord9 discord9 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • Closes none.

Rationale for this change

SortPreservingMergeExec supports fetch, but its statistics currently pass through child statistics unchanged. This can make cost-based optimizers overestimate top-k merge outputs. For example, a SortPreservingMergeExec(fetch=1) side can still look as large as its input to JoinSelection, causing hash join build/probe choices to miss the small top-k side.

Other fetch-aware operators such as SortExec, CoalescePartitionsExec, and limit execs already cap their statistics by fetch. This PR aligns SortPreservingMergeExec with that behavior.

What changes are included in this PR?

  • Cap SortPreservingMergeExec statistics with Statistics::with_fetch(self.fetch, 0, 1).
  • Preserve no-op statistics for fetch = None and skip = 0.
  • Avoid scaling byte-size estimates by 0.0 when input row count is unknown; use absent byte-size stats instead.
  • Add regression tests for SPM fetch statistics and for JoinSelection swapping an SPM(fetch=1) side to the hash join build side.

Are these changes tested?

Yes.

  • cargo test -p datafusion-common test_with_fetch
  • cargo test -p datafusion-physical-plan sort_preserving_merge
  • cargo test -p datafusion join_selection --test core_integration
  • cargo check -p datafusion --test core_integration
  • cargo fmt --all -- --check
  • cargo clippy --all-targets --all-features -- -D warnings

Are there any user-facing changes?

No user-facing API changes. This improves optimizer statistics for fetch-limited sort-preserving merge plans.

@github-actions github-actions Bot added core Core DataFusion crate common Related to common crate physical-plan Changes to the physical-plan crate labels Jul 7, 2026

@kosiew kosiew 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.

Looks good overall. I left one small suggestion to keep the cardinality metadata consistent with the fetch-aware statistics behavior. Can you also resolve the merge conflict?

Comment thread datafusion/physical-plan/src/sorts/sort_preserving_merge.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate core Core DataFusion crate physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants