Refactor: add `FileGroup` structure for `Vec<PartitionedFile>` by xudong963 · Pull Request #15379 · apache/datafusion

xudong963 · 2025-03-24T08:43:14Z

Which issue does this PR close?

related to Epic: Statistics improvements #8227

Rationale for this change

FileGroup encapsulates both files and their statistics, enabling more effective query planning and optimization based on file group characteristics. Moving from raw Vec<PartitionedFile> to a dedicated structure creates a proper abstraction that bundles related functionality together.
We can add the FileGroup's specific API in the future, currently, one that comes into my mind is part of the split_groups_by_statistics method, it make any two files in a file group are ordered.
Make the file_groups clearer, from pub file_groups: Vec<Vec<PartitionedFile>> to pub file_groups: Vec<FileGroup>

What changes are included in this PR?

Introduce FileGroup and implement the related APIs, replace all Vec<PartitionedFile> with FileGroup

Are these changes tested?

Yes

Are there any user-facing changes?

datafusion/datasource/src/file_groups.rs

datafusion/datasource/src/file_sink_config.rs

datafusion/catalog-listing/src/helpers.rs

xudong963 · 2025-03-24T08:59:50Z

datafusion/datasource/src/file_groups.rs

+    /// The files in this group
+    pub files: Vec<PartitionedFile>,
+    /// Optional statistics for all files in the group
+    pub statistics: Option<Statistics>,


We can compute the statistics in the method of ListingTable

datafusion/datafusion/core/src/datasource/listing/table.rs

Lines 1086 to 1091 in 5210a2b

async fn list_files_for_scan<'a>(

&'a self,

ctx: &'a dyn Session,

filters: &'a [Expr],

limit: Option<usize>,

) -> Result<(Vec<Vec<PartitionedFile>>, Statistics)> {

I recommend:

Remove the pub from the fields (so we can potentially change the representation later)

Add a into_inner() method that returns the inner Vec

done: 7415f7c

jayzhan211 · 2025-03-24T12:21:11Z

datafusion/core/src/datasource/statistics.rs

-) -> Result<(Vec<PartitionedFile>, Statistics)> {
-    let mut result_files = vec![];
+) -> Result<(FileGroup, Statistics)> {
+    let mut result_files = FileGroup::default();


What are the difference between the statistics in FileGroup and the returned value here? Do we need statistics in FileGroup, if so, could we add the returned Statistics inside FileGroup?

The returned Statistics here is all files' merged statistics, or we can say it's the FileGroups statistics.

This is an issue that may need the FileGroup's statistic: #10316 and a corresponding PR: #13296

Can we return result_files.with_statistics(statistics)? I don't see any other place where with_statistics is called. The inner optional statistics in FileGroup is a bit confusing to me.

@jayzhan211 , yeah, it's a todo task, I'll compute the FileGroup's statistic in the method and use the with_statistics method to set statistics for FileGroup later.

Fyi: #15432

datafusion/datasource/src/file_stream.rs

alamb

Thank you @xudong963

I think encapsulating Vec<PartitionedFile> into FileGroup is a very nice idea and long in coming. However I do think it is an API change so I have maked this PR with that tag.

I also added this to the list of API changes that should have entries in the upgrade guide on

#15072

alamb · 2025-03-24T19:20:13Z

datafusion/catalog-listing/src/helpers.rs

 const CONCURRENCY_LIMIT: usize = 100;

-/// Partition the list of files into `n` groups
-pub fn split_files(


this is an API change -- can you perhaps leave the function here and mark it #deprecated per https://datafusion.apache.org/contributor-guide/api-health.html#deprecation-guidelines

done 7415f7c

alamb · 2025-03-24T19:23:05Z

datafusion/datasource/src/file_groups.rs

+    /// The files in this group
+    pub files: Vec<PartitionedFile>,
+    /// Optional statistics for all files in the group
+    pub statistics: Option<Statistics>,


I recommend:

Remove the pub from the fields (so we can potentially change the representation later)

Add a into_inner() method that returns the inner Vec

alamb · 2025-03-24T19:24:44Z

datafusion/datasource/src/file_groups.rs

+    }
+
+    /// Partition the list of files into `n` groups
+    pub fn split_files(&mut self, n: usize) -> Vec<FileGroup> {


Rather than taking &mut self here, I think this would be easier to use if it took mut self

As written it will leave self empty which is not super obvious. If it took mut self then it would be clear that the FileGroup is consumed

Yes, makes a lot sense

alamb · 2025-03-24T19:25:15Z

datafusion/datasource/src/file_groups.rs

+        let files = iter.into_iter().collect();
+        FileGroup::new(files)
+    }
+}


I also recommend a From impl for the vec

Suggested change

}

}

impl From<Vec<PartitionedFile>> for FileGroup {

...

}

Yeah, added in 7415f7c

Now I have three ways to create FileGroup by Vec<PartitionedFile>.

let files = vec![...]; let file_group = FileGroup::new(files); let file_group = files.into(); let file_group = FileGroup::from(files);

I don't strongly prefer which method to use, but the second is simpler!

alamb · 2025-03-24T19:25:19Z

datafusion/datasource/src/file_groups.rs

+    }
+
+    /// Partition the list of files into `n` groups
+    pub fn split_files(&mut self, n: usize) -> Vec<FileGroup> {


Rather than taking &mut self here, I think this would be easier to use if it took mut self

As written it will leave self empty which is not super obvious. If it took mut self then it would be clear that the FileGroup is consumed

datafusion/datasource/src/file_groups.rs

xudong963 · 2025-03-25T04:18:39Z

However I do think it is an API change so I have maked this PR with that tag.

I also added this to the list of API changes that should have entries in the upgrade guide on

Yes, I agree. Thank you for adding it.

xudong963 · 2025-03-25T11:17:34Z

Thanks for your review. I'll merge it after @alamb has another look!

alamb · 2025-03-25T17:09:58Z

datafusion/datasource/src/file_groups.rs


    /// Partition the list of files into `n` groups
-    pub fn split_files(&mut self, n: usize) -> Vec<FileGroup> {
+    pub fn split_files(mut self, n: usize) -> Vec<FileGroup> {


jayzhan211 · 2025-03-26T00:44:07Z

Thanks @xudong963 @alamb

…e#15379) * Refactor: add FileGroup structure * update * address comments * address comments from alamb * fix clippy

Refactor: add FileGroup structure

a13e083

github-actions bot added core Core DataFusion crate substrait Changes to the substrait crate proto Related to proto crate datasource Changes to the datasource crate labels Mar 24, 2025

xudong963 commented Mar 24, 2025

View reviewed changes

datafusion/datasource/src/file_groups.rs Show resolved Hide resolved

datafusion/datasource/src/file_sink_config.rs Show resolved Hide resolved

xudong963 commented Mar 24, 2025

View reviewed changes

datafusion/catalog-listing/src/helpers.rs Outdated Show resolved Hide resolved

xudong963 commented Mar 24, 2025

View reviewed changes

xudong963 force-pushed the add_file_group branch from cda1c47 to 1dc5153 Compare March 24, 2025 09:04

update

7df36ed

xudong963 force-pushed the add_file_group branch from 1dc5153 to 7df36ed Compare March 24, 2025 09:23

jayzhan211 reviewed Mar 24, 2025

View reviewed changes

address comments

04425f1

alamb added the api change Changes the API exposed to users of the crate label Mar 24, 2025

alamb mentioned this pull request Mar 24, 2025

Release DataFusion 47.0.0 (April 2025) #15072

Closed

39 tasks

alamb approved these changes Mar 24, 2025

View reviewed changes

alamb changed the title ~~Refactor: add FileGroup structure~~ Refactor: add FileGroup structure for Vec<PartitionedFile> Mar 24, 2025

address comments from alamb

7415f7c

fix clippy

4ee4253

jayzhan211 approved these changes Mar 25, 2025

View reviewed changes

alamb approved these changes Mar 25, 2025

View reviewed changes

jayzhan211 merged commit de8bcc5 into apache:main Mar 26, 2025
28 checks passed

xudong963 mentioned this pull request Mar 26, 2025

Support computing statistics for FileGroup #15432

Merged

xudong963 deleted the add_file_group branch March 26, 2025 08:59

alamb mentioned this pull request Apr 1, 2025

[EPIC] Avoid sort for already sorted Parquet files that do not overlap values on condition #6672

Open

11 tasks

alamb mentioned this pull request Apr 14, 2025

Upgrade guide for DataFusion 47.0.0 #15707

Closed

9 tasks

nirnayroy pushed a commit to nirnayroy/datafusion that referenced this pull request May 2, 2025

Refactor: add FileGroup structure for Vec<PartitionedFile> (apach…

809c48d

…e#15379) * Refactor: add FileGroup structure * update * address comments * address comments from alamb * fix clippy

	async fn list_files_for_scan<'a>(
	&'a self,
	ctx: &'a dyn Session,
	filters: &'a [Expr],
	limit: Option<usize>,
	) -> Result<(Vec<Vec<PartitionedFile>>, Statistics)> {

-}
+}
+impl From<Vec<PartitionedFile>> for FileGroup {
+...
+}

Comments

Conversation

xudong963 commented Mar 24, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xudong963 Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xudong963 Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xudong963 commented Mar 25, 2025

Uh oh!

xudong963 commented Mar 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jayzhan211 commented Mar 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xudong963 commented Mar 24, 2025 •

edited by alamb

Loading

jayzhan211 Mar 24, 2025 •

edited

Loading

xudong963 Mar 25, 2025 •

edited

Loading

xudong963 Mar 25, 2025 •

edited

Loading