Uh oh!
There was an error while loading. Please reload this page.
Improve sqllogicteset speed by creating only a single large file rather than 2 - #20586
Merged
alamb merged 1 commit intoMar 2, 2026
Merged
Conversation
alamb
commented
Feb 27, 2026
Contributor
Thank you 🙏 I left a note on Asking the original authors if they could double check |
| } | ||
| } | ||
| // trigger ci test |
Contributor
There was a problem hiding this comment.
Can be removed? Also just in case it's helpful: git commit -m "ci" --allow-empty --no-verify
Contributor
There was a problem hiding this comment.
(I think this is left over from #20566 -- when this PR gets rebased it should be removed)
adriangb
left a comment
Contributor
There was a problem hiding this comment.
I don't think the test needs two physically distinct files. As long as it's two different execution nodes that should be good enough!
alambforce-pushed
the
perf/reuse-parquet-file-push-down-filter-regression
branch
from
March 1, 2026 13:40
acce9a4 to
6a36b9eComparealamb
approved these changes
Mar 1, 2026
alamb
left a comment
Contributor
There was a problem hiding this comment.
I took the liberty of rebasing this PR against main.
I think it looks good to me
alamb
marked this pull request as ready for review
March 1, 2026 13:41
alamb
approved these changes
Mar 1, 2026
alamb
left a comment
Contributor
There was a problem hiding this comment.
I took the liberty of rebasing this PR against main.
I think it looks good to me
alamb
commented
Mar 2, 2026
Contributor
Thanks again @Tim-53 |
Uh oh!
There was an error while loading. Please reload this page.
github-merge-queueBot
pushed a commit
that referenced
this pull request
Mar 4, 2026
…#20652) ## Which issue does this PR close? - Closes#20524 ## Rationale for this change `push_down_filter_regression.slt ` is the sqllogictest that takes the longest to run, even after @Tim-53 reduced its time in - #20586 While reviewing #20586 and trying to make the sqllogictest runs faster, I noticed that a substantial amount of the unit test time was spent doing zstd compression/decompression: <img width="2423" height="841" alt="Screenshot 2026-03-02 at 12 50 24 PM" src="https://github.com/user-attachments/assets/75cfe12b-3bb2-4ffa-9c36-63ca00b8c3ff" /> Thus, we can improve the test speed by skipping the zstd step ## What changes are included in this PR? 1. Don't compress the parquet files in the test ## Are these changes tested? Yes by CI Here are my performance runs using @kosiew 's new timing feature ```shell cargo test --profile=ci --test sqllogictests -- --timing-summary top ``` Main: ``` Per-file elapsed summary (deterministic): 1. 4.035s push_down_filter_regression.slt <-- takes over 4 seconds 2. 3.573s joins.slt 3. 3.492s aggregate.slt 4. 3.316s imdb.slt 5. ``` This PR ``` Per-file elapsed summary (deterministic): 1. 3.308s aggregate.slt 2. 3.290s joins.slt 3. 3.181s imdb.slt 4. 2.914s push_down_filter_regression.slt <--- takes less than 3 seconds and is no longer the tallest pole ``` ## Are there any user-facing changes? Faster tests
de-bgunter pushed a commit
to de-bgunter/datafusion
that referenced
this pull request
Mar 24, 2026
…er than 2 (apache#20586) Draft as it builds on apache#20576 ## Which issue does this PR close? - Part of apache#20524 - Follow on to apache#20576 from @alamb ## Rationale for this change Execution time of the test is dominated by the time writing the parquet files. By reusing the file we can gain around 30% improvement on the execution time here. ## What changes are included in this PR? Building on apache#20576 we reuse the needed parquet file for the test instead of recreating it. ## Are these changes tested? Ran the test with following results: | | Baseline (2 files) | Optimized (1 file) | |---|---|---| | Min | 33.000s | 22.653s | | Max | 37.662s | 25.489s | | Avg | 34.427s | 24.092s | One open question: does the correctness of this regression test rely on having two **physically separate** files? The race condition in apache#17197 was in the execution layer — both scans would still be independent `DataSourceExec` nodes with independent readers, so I believe the behavior is preserved. But if there's any concern, we could use `system cp` to copy the file and register two physical files while still only paying the `generate_series` cost once. ## Are there any user-facing changes?
de-bgunter pushed a commit
to de-bgunter/datafusion
that referenced
this pull request
Mar 24, 2026
…apache#20652) ## Which issue does this PR close? - Closesapache#20524 ## Rationale for this change `push_down_filter_regression.slt ` is the sqllogictest that takes the longest to run, even after @Tim-53 reduced its time in - apache#20586 While reviewing apache#20586 and trying to make the sqllogictest runs faster, I noticed that a substantial amount of the unit test time was spent doing zstd compression/decompression: <img width="2423" height="841" alt="Screenshot 2026-03-02 at 12 50 24 PM" src="https://github.com/user-attachments/assets/75cfe12b-3bb2-4ffa-9c36-63ca00b8c3ff" /> Thus, we can improve the test speed by skipping the zstd step ## What changes are included in this PR? 1. Don't compress the parquet files in the test ## Are these changes tested? Yes by CI Here are my performance runs using @kosiew 's new timing feature ```shell cargo test --profile=ci --test sqllogictests -- --timing-summary top ``` Main: ``` Per-file elapsed summary (deterministic): 1. 4.035s push_down_filter_regression.slt <-- takes over 4 seconds 2. 3.573s joins.slt 3. 3.492s aggregate.slt 4. 3.316s imdb.slt 5. ``` This PR ``` Per-file elapsed summary (deterministic): 1. 3.308s aggregate.slt 2. 3.290s joins.slt 3. 3.181s imdb.slt 4. 2.914s push_down_filter_regression.slt <--- takes less than 3 seconds and is no longer the tallest pole ``` ## Are there any user-facing changes? Faster tests
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
3 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.
Draft as it builds on #20576
Which issue does this PR close?
Rationale for this change
Execution time of the test is dominated by the time writing the parquet files. By reusing the file we can gain around 30% improvement on the execution time here.
What changes are included in this PR?
Building on #20576 we reuse the needed parquet file for the test instead of recreating it.
Are these changes tested?
Ran the test with following results:
One open question: does the correctness of this regression test rely on having two physically separate files? The race condition in #17197 was in the execution layer — both scans would still be independent
DataSourceExecnodes with independent readers, so I believe the behavior is preserved. But if there's any concern, we could usesystem cpto copy the file and register two physical files while still only paying thegenerate_seriescost once.Are there any user-facing changes?