Repository navigation
Return provider extensions directly from the statistics walk - #25826
Merged
Merged
Conversation
- Update `StatisticsContext` to pass extensions from providers into core statistics - Modify bottom-up walk to return `(statistics, extensions)` from provider results - Ensure cache retains provider-attached extensions even in partition-blind cases
- Split `into_parts` to return base and extensions without cloning, improving efficiency. - Update `StatisticsContext` to handle extension caching more precisely, avoiding redundant clones. - Modify provider extension propagation to avoid cache-based cloning when possible, improving performance.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25826 +/- ##
==========================================
- Coverage 82.58% 82.57% -0.01%
==========================================
Files 1142 1142
Lines 440930 440940 +10
Branches 440930 440940 +10
==========================================
- Hits 364126 364124 -2
- Misses 54801 54811 +10
- Partials 22003 22005 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
asolimando
approved these changes
Sep 28, 2026
asolimando
left a comment
asolimando
left a comment
Member
There was a problem hiding this comment.
LGTM, thanks @kosiew for this improvement, it's good to not pay the price of cloning extensions unless needed!
kosiew
marked this pull request as ready for review
September 29, 2026 13:33
kosiew
force-pushed
the
cacherobust-01-25141
branch
from
September 29, 2026 13:34
39b0e20 to
c27f226
Compare
Contributor
Author
|
@asolimando |
kosiew
enabled auto-merge
September 30, 2026 01:49
kosiew
disabled auto-merge
September 30, 2026 01:50
zhuqi-lucas
approved these changes
Sep 30, 2026
zhuqi-lucas
left a comment
zhuqi-lucas
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thanks @kosiew !
Contributor
Author
|
🚀 |
Omega359
pushed a commit
to Omega359/arrow-datafusion
that referenced
this pull request
Oct 11, 2026
…25826) ## Which issue does this PR close? Part of apache#25141. ## Rationale for this change `compute_extended` currently reads a root node’s provider extensions back from the cache after computing its statistics. A planned change will stop caching borrowed roots, so that read would lose their extensions. Returning the extensions from the statistics walk preserves the result without depending on a root cache entry. ## What changes are included in this PR? - Return statistics and provider extensions together from the private statistics walk. `compute_extended` uses that result; `compute` keeps returning only the statistics. - Continue caching provider extensions for parent nodes. On a cache hit, read them only when the caller requests them. - Replace `ExtendedStatistics::extensions()` with `into_parts()` to move its statistics and extensions into the walk’s result. The public signatures of `compute` and `compute_extended` are unchanged. ## Are these changes tested? The patch adds `cached_node_keeps_extensions` and `walk_returns_provider_extensions_directly`. They cover extensions returned after a child is cached, extensions returned from a fresh provider result, and cache reads that occur only when requested. ## Are there any user-facing changes? No user-facing change is intended. This refactor preserves provider extension behavior ahead of the planned cache change. ## LLM-generated code disclosure This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #25141.
Rationale for this change
compute_extendedcurrently reads a root node’s provider extensions back from the cache after computing its statistics. A planned change will stop caching borrowed roots, so that read would lose their extensions. Returning the extensions from the statistics walk preserves the result without depending on a root cache entry.What changes are included in this PR?
compute_extendeduses that result;computekeeps returning only the statistics.ExtendedStatistics::extensions()withinto_parts()to move its statistics and extensions into the walk’s result.The public signatures of
computeandcompute_extendedare unchanged.Are these changes tested?
The patch adds
cached_node_keeps_extensionsandwalk_returns_provider_extensions_directly. They cover extensions returned after a child is cached, extensions returned from a fresh provider result, and cache reads that occur only when requested.Are there any user-facing changes?
No user-facing change is intended. This refactor preserves provider extension behavior ahead of the planned cache change.
LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.