Uh oh!
There was an error while loading. Please reload this page.
Move statistics handling into FileScanConfig - #18721
Merged
Merged
Conversation
adriangbforce-pushed
the
projection-source-stats
branch
from
November 15, 2025 00:49
b84171f to
e2d7126Compareadriangb
marked this pull request as draft
November 15, 2025 00:50
adriangbforce-pushed
the
projection-source-stats
branch
from
November 15, 2025 07:14
e2d7126 to
5ebc343Compareadriangb
marked this pull request as ready for review
November 15, 2025 07:16
adriangb
commented
Nov 15, 2025
Comment on lines
+747
to
+753
| pub fn statistics(&self) -> Statistics { | ||
| if self.file_source.filter().is_some() { | ||
| self.statistics.clone().to_inexact() | ||
| } else { | ||
| self.statistics.clone() | ||
| } | ||
| } |
ContributorAuthor
There was a problem hiding this comment.
Now we don't have to rely on each FileSource to remember to do this 😄
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
adriangbforce-pushed
the
projection-source-stats
branch
from
November 17, 2025 22:26
317486b to
c672a73Compareadriangbforce-pushed
the
projection-source-stats
branch
2 times, most recently
from
November 17, 2025 22:30
2dbfd1e to
ccb4e69Comparemartin-g
approved these changes
Nov 18, 2025
xudong963
self-requested a review
November 18, 2025 09:46
adriangb
enabled auto-merge
November 18, 2025 12:50
adriangb
disabled auto-merge
November 18, 2025 13:02
adriangbforce-pushed
the
projection-source-stats
branch
from
November 19, 2025 10:39
a9c8746 to
0ad47a0Compareleoyvens
approved these changes
Nov 19, 2025
leoyvens
left a comment
Contributor
There was a problem hiding this comment.
I'm not a maintainer, but LGTM
adriangbforce-pushed
the
projection-source-stats
branch
from
November 19, 2025 22:42
0ad47a0 to
dc3e9dbComparealamb
approved these changes
Nov 20, 2025
| } | ||
| } | ||
| pub fn projected_stats(&self) -> Statistics { |
Contributor
There was a problem hiding this comment.
I think this is a (small) public API change, so we should flag this PR thusly (I did) and add a note to the upgrade guide
| pub expr_adapter_factory: Option<Arc<dyn PhysicalExprAdapterFactory>>, | ||
| /// Unprojected statistics for the table (file schema + partition columns). | ||
| /// These are projected on-demand via `projected_stats()`. | ||
| pub(crate) statistics: Statistics, |
Contributor
There was a problem hiding this comment.
nit is that this field has a different visibility (pub(crate)) than all the other fields
| write!( | ||
| f, | ||
| "statistics={:?}, ", | ||
| self.file_source.statistics().unwrap() |
Contributor
There was a problem hiding this comment.
removing an unwrap in the display certainly seems like an improvement to me
Uh oh!
There was an error while loading. Please reload this page.
logan-keede pushed a commit
to logan-keede/datafusion
that referenced
this pull request
Nov 23, 2025
## Summary This PR moves statistics handling from individual `FileSource` implementations into `FileScanConfig`, simplifying the `FileSource` trait interface. The `FileSource`s were all acting as a container for the statistics but never actually using them. Since `FileScanConfig` deals with file-level things (which the statistics are) it is better equipped to deal with it. ### Changes - **FileSource trait simplification**: Removed `statistics()`, `with_statistics()`, and `with_projection()` methods - **FileScanConfig enhancement**: Added `statistics` field and `statistics()` method - **FileSource implementations updated**: Removed `projected_statistics` field from all implementations: - ParquetSource - CsvSource - JsonSource - AvroSource - ArrowFileSource and ArrowStreamFileSource - MockSource (test utility) - **Test utilities**: Updated assertions to use `config.statistics()` instead of `file_source.statistics()` - **Proto serialization**: Updated to use `config.statistics()` ### Benefits 1. **Simpler trait interface**: `FileSource` implementations no longer need to manage statistics 2. **Centralized statistics**: All statistics are now managed consistently in `FileScanConfig` 3. **Cleaner API**: Statistics lifecycle is clearer and less error-prone 4. **Reduced code duplication**: Removes ~140 lines of boilerplate across implementations ### Related This is part of the projection refactoring work in apache#18627. This PR extracts just the statistics-related changes to make review easier. The full projection refactoring will come in subsequent PRs. ## Test plan - [x] All modified file source implementations compile - [x] Test utilities updated and compile - [x] CI tests pass (will verify after PR creation) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Summary
This PR moves statistics handling from individual
FileSourceimplementations intoFileScanConfig, simplifying theFileSourcetrait interface.The
FileSources were all acting as a container for the statistics but never actually using them.Since
FileScanConfigdeals with file-level things (which the statistics are) it is better equipped to deal with it.Changes
statistics(),with_statistics(), andwith_projection()methodsstatisticsfield andstatistics()methodprojected_statisticsfield from all implementations:config.statistics()instead offile_source.statistics()config.statistics()Benefits
FileSourceimplementations no longer need to manage statisticsFileScanConfigRelated
This is part of the projection refactoring work in #18627. This PR extracts just the statistics-related changes to make review easier. The full projection refactoring will come in subsequent PRs.
Test plan
🤖 Generated with Claude Code