Skip to content

Split FileStreamMetrics into its own module - #21340

Merged
comphead merged 3 commits into
apache:mainfrom
alamb:alamb/move_file_scan_metrics
Apr 3, 2026
Merged

Split FileStreamMetrics into its own module#21340
comphead merged 3 commits into
apache:mainfrom
alamb:alamb/move_file_scan_metrics

Conversation

@alamb

@alambalamb commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

@Dandandan, @adriangb and I are in the process of significantly reworking how FileStream works internally (morsels!)

As the FileStream will get more complicated, it needs a better structure than one giant module to ensure we can work with it. I am trying to do this as incremental PRs to keep review burden low

What changes are included in this PR?

  1. Move FileStreamMetrics to its own module

Are these changes tested?

Yes by CI

Are there any user-facing changes?

No, just internal code motion

@github-actionsgithub-actionsBot added the datasource Changes to the datasource crate label Apr 3, 2026
alamb
alamb commented Apr 3, 2026
View reviewed changes
Comment threaddatafusion/datasource/src/file_stream/mod.rs
}

/// A timer that can be started and stopped.
pub struct StartableTime {

@alambalambApr 3, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is just moved into its own module in the first commit

I also had to touch up the links in the documentation which I did in its own commit for easier review

@alamb
alamb marked this pull request as ready for review April 3, 2026 13:19
This was referenced Apr 3, 2026
Closed
Merged
Merged
comphead
comphead approved these changes Apr 3, 2026
View reviewed changes

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

Thanks, makes a lot of sense

@comphead
comphead added this pull request to the merge queueApr 3, 2026
Merged via the queue into apache:main with commit e1c5453Apr 3, 2026
31 checks passed
@alamb

alamb commented Apr 6, 2026

Copy link
Copy Markdown
ContributorAuthor

Thanks @comphead

github-merge-queueBot pushed a commit that referenced this pull request Apr 14, 2026
@alamb@adriangb
Rewrite FileStream in terms of Morsel API (#21342)
776b723
Stacked on
- #21327
- #21340
## Which issue does this PR close?
- part of #20529
- Broken out of #20820
## Rationale for this change
The Morsel API allows for finer grain parallelism (and IO). It is
important to have the FileStream work in terms of the Morsel API to
allow future features (like workstealing, etc)
## What changes are included in this PR?
I apologize for the large diff; Note about 1/2 of this PR is tests and a
test framework to test the calling sequence of FileStream.
1. Rewrite FileStream in terms of the MorselAPI
2. Add snapshot driven test to document the I/O and CPU patterns in
FileStream
3. Add snapshot based tests that show the ordering of files
## Are these changes tested?
Yes by existing functional and benchmark tests, as well as new
functional snapshot based tests
## Are there any user-facing changes?
No (not yet)
---------
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
github-merge-queueBot pushed a commit that referenced this pull request Apr 20, 2026
@alamb@comphead
Dynamic work scheduling in FileStream (#21351)
466c3ea
## Which issue does this PR close?
- Closes#20529
- Closes#20820
## Rationale for this change
This PR finally enables dynamic work scheduling in the FileStream (so
that if a task is done it can look at any remaining work)
This improves performance on queries that scan multiple files and the
work is not balanced evenly across partitions in the plan (e.g. we have
dynamic filtering that reduces work significantly)
It is the last of a sequence of several PRs:
- #21342
- #21327
- #21340
## What changes are included in this PR?
1. Add shared state across sibling FileStream's and the wiring to
connect them
2. Sibling streams put their file work into a shared queue when it can
be reordered
3. Add a bunch of tests sjpw
Note there are a bunch of other things that are NOT included in this PR,
including
1. Trying to limit concurrent IO (this PR has the same properties as
main -- up to one outstanding IO per partition)
2. Trying to issue multiple IOs by the same partition (aka to interleave
IO and CPU work more)
4. Splitting files into smaller units (e.g. across row groups)
As @Dandandan proposes below, I expect we can work on those changes as
follow on PRs.
## Are these changes tested?
Yes by existing functional and benchmark tests, as well as new
functional tests
## Are there any user-facing changes?
Yes, faster performance (see benchmarks):
#21351 (comment)
---------
Co-authored-by: Oleks V <comphead@users.noreply.github.com>
Rich-T-kid pushed a commit to Rich-T-kid/datafusion that referenced this pull request Apr 21, 2026
@alamb@Rich-T-kid
Split FileStreamMetrics into its own module (apache#21340)
9c1881f
## Which issue does this PR close?
- part of apache#20529
- Broken out of apache#20820
## Rationale for this change
@Dandandan, @adriangb and I are in the process of significantly
reworking how FileStream works internally (morsels!)
As the FileStream will get more complicated, it needs a better structure
than one giant module to ensure we can work with it. I am trying to do
this as incremental PRs to keep review burden low
## What changes are included in this PR?
1. Move FileStreamMetrics to its own module
## Are these changes tested?
Yes by CI
## Are there any user-facing changes?
No, just internal code motion
Rich-T-kid pushed a commit to Rich-T-kid/datafusion that referenced this pull request Apr 21, 2026
@alamb@adriangb@Rich-T-kid
Rewrite FileStream in terms of Morsel API (apache#21342)
462ee99
Stacked on
- apache#21327
- apache#21340
## Which issue does this PR close?
- part of apache#20529
- Broken out of apache#20820
## Rationale for this change
The Morsel API allows for finer grain parallelism (and IO). It is
important to have the FileStream work in terms of the Morsel API to
allow future features (like workstealing, etc)
## What changes are included in this PR?
I apologize for the large diff; Note about 1/2 of this PR is tests and a
test framework to test the calling sequence of FileStream.
1. Rewrite FileStream in terms of the MorselAPI
2. Add snapshot driven test to document the I/O and CPU patterns in
FileStream
3. Add snapshot based tests that show the ordering of files
## Are these changes tested?
Yes by existing functional and benchmark tests, as well as new
functional snapshot based tests
## Are there any user-facing changes?
No (not yet)
---------
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Rich-T-kid pushed a commit to Rich-T-kid/datafusion that referenced this pull request Apr 21, 2026
@alamb@comphead@Rich-T-kid
Dynamic work scheduling in FileStream (apache#21351)
eeb118e
## Which issue does this PR close?
- Closesapache#20529
- Closesapache#20820
## Rationale for this change
This PR finally enables dynamic work scheduling in the FileStream (so
that if a task is done it can look at any remaining work)
This improves performance on queries that scan multiple files and the
work is not balanced evenly across partitions in the plan (e.g. we have
dynamic filtering that reduces work significantly)
It is the last of a sequence of several PRs:
- apache#21342
- apache#21327
- apache#21340
## What changes are included in this PR?
1. Add shared state across sibling FileStream's and the wiring to
connect them
2. Sibling streams put their file work into a shared queue when it can
be reordered
3. Add a bunch of tests sjpw
Note there are a bunch of other things that are NOT included in this PR,
including
1. Trying to limit concurrent IO (this PR has the same properties as
main -- up to one outstanding IO per partition)
2. Trying to issue multiple IOs by the same partition (aka to interleave
IO and CPU work more)
4. Splitting files into smaller units (e.g. across row groups)
As @Dandandan proposes below, I expect we can work on those changes as
follow on PRs.
## Are these changes tested?
Yes by existing functional and benchmark tests, as well as new
functional tests
## Are there any user-facing changes?
Yes, faster performance (see benchmarks):
apache#21351 (comment)
---------
Co-authored-by: Oleks V <comphead@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@compheadcompheadcomphead approved these changes

Assignees

No one assigned

Labels

datasourceChanges to the datasource crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alamb@comphead