Skip to content

fix: report LowerEqual cardinality effect for CoalescePartitionsExec with fetch - #26051

Merged
alamb merged 1 commit into
apache:mainfrom
asolimando:asolimando/coalesce-partitions-fetch-cardinality
Oct 9, 2026
Merged

alamb merged 1 commit into
apache:mainfrom
asolimando:asolimando/coalesce-partitions-fetch-cardinality

Conversation

@asolimando

@asolimando asolimando commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

CoalescePartitionsExec::cardinality_effect returns CardinalityEffect::Equal even when fetch is set, although the operator can then produce fewer rows than its input. Code that relies on cardinality_effect gets the wrong answer, for example PassthroughStatisticsProvider reports the input row count and drops the fetch.

What changes are included in this PR?

  • CoalescePartitionsExec::cardinality_effect returns CardinalityEffect::LowerEqual when fetch is set, as SortExec and SortPreservingMergeExec already do.

What is the testing strategy for this PR?

  • New unit test for cardinality_effect with and without fetch.
  • New unit test showing that PassthroughStatisticsProvider no longer drops the fetch (Exact(10) instead of Exact(1000)).
  • sqllogictest and the core physical_optimizer integration tests pass with no plan changes.

Are there any user-facing changes?

No.


Disclaimer: I used AI to assist in the code generation, I have manually reviewed the output and it matches my intention and understanding.

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Oct 5, 2026
…with fetch

`CoalescePartitionsExec::cardinality_effect` returned `Equal` even when a
fetch is set, so `PassthroughStatisticsProvider` matched it and copied the
child statistics, discarding the fetch that the operator's own
`statistics_from_inputs` applies. Return `LowerEqual` when a fetch is set,
as `SortExec` and `SortPreservingMergeExec` already do.
@asolimando
asolimando force-pushed the asolimando/coalesce-partitions-fetch-cardinality branch from e63ad80 to a170053 Compare October 5, 2026 11:39
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.95652% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.66%. Comparing base (c3ef346) to head (a170053).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...atafusion/physical-plan/src/coalesce_partitions.rs 83.33% 0 Missing and 2 partials ⚠️
...usion/physical-plan/src/operator_statistics/mod.rs 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #26051   +/-   ##
=======================================
  Coverage   82.66%   82.66%           
=======================================
  Files        1147     1147           
  Lines      446357   446452   +95     
  Branches   446357   446452   +95     
=======================================
+ Hits       368971   369055   +84     
+ Misses      54997    54993    -4     
- Partials    22389    22404   +15     

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

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

I traced the cardinality declaration through PassthroughStatisticsProvider and the operator statistics fallback. LowerEqual correctly prevents passthrough when fetch can reduce the output, allowing the existing fetch-aware calculation to cap the estimate.

The new tests cover both declaration branches and the resulting Exact(10) estimate from an Exact(1000) input. Restoring the previous Equal behavior makes the provider regression fail with Exact(1000). The focused physical-plan, optimizer, statistics, SQL logic, formatting, and Clippy checks pass locally. No blocking issues found.

@asolimando

Copy link
Copy Markdown
Member Author

Hey @nuno-faria, thanks a lot for the approval! Is there anything pending or we can merge this?

@alamb
alamb added this pull request to the merge queue Oct 9, 2026
@alamb

alamb commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

I put it in the queue

Tanks @nuno-faria and @asolimando and @rgbuilds

Merged via the queue into apache:main with commit 3310fd5 Oct 9, 2026
42 checks passed
@asolimando
asolimando deleted the asolimando/coalesce-partitions-fetch-cardinality branch October 9, 2026 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CoalescePartitionsExec::cardinality_effect returns Equal when fetch is set

5 participants