Uh oh!
There was an error while loading. Please reload this page.
Remove file prefetching from FileStream - #20916
Conversation
Simplify the FileStream state machine by removing the mechanism that opens the next file in parallel while scanning the current one. Files are now opened sequentially (Scan -> Idle -> Open) instead of prefetching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dandandan
commented
Mar 12, 2026
run benchmarks |
adriangbot
commented
Mar 12, 2026
Benchmark job started for this request (job |
adriangbot
commented
Mar 12, 2026
Benchmark job started for this request (job |
adriangbot
commented
Mar 12, 2026
Benchmark job started for this request (job |
adriangbot
commented
Mar 12, 2026
adriangbot
commented
Mar 12, 2026
adriangbot
commented
Mar 12, 2026
This reverts commit 38fe60a.
adriangbot
commented
Mar 12, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usagetpch — base (merge-base)
tpch — branch
|
adriangbot
commented
Mar 12, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
|
adriangbot
commented
Mar 12, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usagetpcds — base (merge-base)
tpcds — branch
|
| .metrics | ||
| .clone(); | ||
| let _timer = scanning_total_metric.timer(); | ||
| self.start_next_file().transpose() |
There was a problem hiding this comment.
So this @alamb is what I was mostly talking about. It will read the footer (what we want) but AFAIK also:
- build the pruning predicate (I think this is suboptimal, too early)
- prune row groups
- optionally load the page index
- return the stream (without driving that forward)
We should be able to do this much better with the IO / CPU separation.
alamb-ghbot
commented
Mar 12, 2026
🤖 |
alamb-ghbot
commented
Mar 12, 2026
🤖: Benchmark completed Details |
Dandandan
commented
Mar 13, 2026
run benchmarks |
adriangbot
commented
Mar 13, 2026
Benchmark job started for this request (job |
adriangbot
commented
Mar 13, 2026
Benchmark job started for this request (job |
adriangbot
commented
Mar 13, 2026
Benchmark job started for this request (job |
adriangbot
commented
Mar 13, 2026
adriangbot
commented
Mar 13, 2026
adriangbot
commented
Mar 13, 2026
thinkharderdev
left a comment
There was a problem hiding this comment.
Makes sense. The original goal here was to pipeline the IO but current file opening blends too much IO and CPU work for this to be effective I think
Dandandan
commented
Mar 17, 2026
run benchmark supported? |
adriangbot
commented
Mar 17, 2026
Hi @Dandandan, thanks for the request (#20916 (comment)). Supported benchmarks:
Usage: Per-side configuration ( env:
SHARED_SETTING: enabledbaseline:
ref: v45.0.0env:
DATAFUSION_RUNTIME_MEMORY_LIMIT: 1Gchanged:
ref: v46.0.0env:
DATAFUSION_RUNTIME_MEMORY_LIMIT: 2G |
Dandandan
commented
Mar 17, 2026
run benchmark tpch10 clickbench_partitioned clickbench_1 clickbench_extended |
adriangbot
commented
Mar 17, 2026
adriangbot
commented
Mar 17, 2026
adriangbot
commented
Mar 17, 2026
adriangbot
commented
Mar 17, 2026
Dandandan
commented
Mar 17, 2026
Thanks all, let's wait a couple of days for further feedback - I launched some more benchmarks to check them all |
Dandandan
commented
Mar 17, 2026
run benchmark tpch10 clickbench_partitioned clickbench_1 clickbench_extended |
adriangbot
commented
Mar 17, 2026
adriangbot
commented
Mar 17, 2026
adriangbot
commented
Mar 17, 2026
adriangbot
commented
Mar 17, 2026
adriangbot
commented
Mar 17, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usagetpch10 — base (merge-base)
tpch10 — branch
|
adriangbot
commented
Mar 17, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usageclickbench_extended — base (merge-base)
clickbench_extended — branch
|
adriangbot
commented
Mar 17, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
|
adriangbot
commented
Mar 17, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usageclickbench_extended — base (merge-base)
clickbench_extended — branch
|
adriangbot
commented
Mar 17, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usageclickbench_1 — base (merge-base)
clickbench_1 — branch
|
adriangbot
commented
Mar 17, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usageclickbench_1 — base (merge-base)
clickbench_1 — branch
|
adriangbot
commented
Mar 17, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
|
adriangbot
commented
Mar 17, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usagetpch10 — base (merge-base)
tpch10 — branch
|
Dandandan
commented
Mar 20, 2026
Let's go forward with this! |
Uh oh!
There was an error while loading. Please reload this page.
Simplify the FileStream state machine by removing the mechanism that opens the next file in parallel while scanning the current one. Files are now opened sequentially (Scan -> Idle -> Open) instead of prefetching. ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closesapache#123` indicates that this PR will close issue apache#123. --> - Closesapache#20984 ## Rationale for this change Current prefetching is ineffective or even counterproductive, see e.g. query 6 when testing against simulated S3 latency: ``` │ QQuery 6 │ 709.93 / 744.83 ±21.29 / 769.15 ms │ 304.17 / 436.76 ±90.12 / 542.71 ms │ +1.71x faster │ ``` Let's remove it so we can make room for better prefetching taking care of pruning / late materilization better, cache behavior and IO/CPU split. ## What changes are included in this PR? Remove this API / prefetching to make room for more impactful prefetching, ## Are these changes tested? Existing tests. ## Are there any user-facing changes? Yes, `NextOpen` `next: Option<NextOpen>,` are removed of `pub` members. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Simplify the FileStream state machine by removing the mechanism that opens the next file in parallel while scanning the current one. Files are now opened sequentially (Scan -> Idle -> Open) instead of prefetching. ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closesapache#123` indicates that this PR will close issue apache#123. --> - Closesapache#20984 ## Rationale for this change Current prefetching is ineffective or even counterproductive, see e.g. query 6 when testing against simulated S3 latency: ``` │ QQuery 6 │ 709.93 / 744.83 ±21.29 / 769.15 ms │ 304.17 / 436.76 ±90.12 / 542.71 ms │ +1.71x faster │ ``` Let's remove it so we can make room for better prefetching taking care of pruning / late materilization better, cache behavior and IO/CPU split. ## What changes are included in this PR? Remove this API / prefetching to make room for more impactful prefetching, ## Are these changes tested? Existing tests. ## Are there any user-facing changes? Yes, `NextOpen` `next: Option<NextOpen>,` are removed of `pub` members. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Simplify the FileStream state machine by removing the mechanism that opens the next file in parallel while scanning the current one. Files are now opened sequentially (Scan -> Idle -> Open) instead of prefetching.
Which issue does this PR close?
Rationale for this change
Current prefetching is ineffective or even counterproductive, see e.g. query 6 when testing against simulated S3 latency:
Let's remove it so we can make room for better prefetching taking care of pruning / late materilization better, cache behavior and IO/CPU split.
What changes are included in this PR?
Remove this API / prefetching to make room for more impactful prefetching,
Are these changes tested?
Existing tests.
Are there any user-facing changes?
Yes,
NextOpennext: Option<NextOpen>,are removed ofpubmembers.