Super fast extended tests and improved planning speed linux#21084
Merged
Super fast extended tests and improved planning speed linux#21084
Conversation
github-actions
bot
added
development-process
blaginin
commented
Mar 20, 2026
| submodules: true | ||
| fetch-depth: 1 | ||
| - name: Setup Rust toolchain | ||
| uses: ./.github/actions/setup-builder |
Member
Author
There was a problem hiding this comment.
this sets RUST_BACKTRACE=1 and we have a lot of expected failures in sqlogictests - getting backtrace for those is pointless and expensive
Contributor
There was a problem hiding this comment.
I recommend adding a comment here explaining the rationale for not calling setup-builder -- otherwise someone in the future could very reasonably undo this optimization by accident I think
blaginin
commented
Mar 20, 2026
| available_parallelism() | ||
| .unwrap_or(NonZero::new(1).expect("literal value `1` shouldn't be zero")) | ||
| .get() | ||
| static PARALLELISM: LazyLock<usize> = LazyLock::new(|| { |
Member
Author
There was a problem hiding this comment.
when running on linux, this function is constantly running
Member
Author
There was a problem hiding this comment.
this obviously affects not only tests, happy to apply to only tests, but feels like a good change overall? 🤔
Contributor
There was a problem hiding this comment.
Hmm great find!
Perhaps there also seems to go something wrong with rewrite_with_subqueries + SimplifyExpressions that make it call so often / create it so often.
Contributor
There was a problem hiding this comment.
I think part of it is that SimplifyContext doesn't have a builder, we end up allocating its internal fields (which includes ConfigOptions, which is what ends up calling get_available_parallelism like @blaginin pointed out here), then immediately overriding the config and schema.
Contributor
There was a problem hiding this comment.
I agree -- it would be great if we could solve that and thread the correct context down. Maybe as a follow on PR?
Contributor
There was a problem hiding this comment.
Maybe something like this will also help #21092
blaginin
commented
Mar 20, 2026
Cargo.toml
Outdated
blaginin
force-pushed
the
db/cache-cores
branch
from
4ad299f to
34d3034
Compare
Contributor
|
Run benchmark sql_planner |
|
🤖 Criterion benchmark running (GKE) | trigger |
|
Benchmark for this request failed. Last 20 lines of output: Click to expand |
alamb
reviewed
Mar 21, 2026
alamb
changed the title
Contributor
|
BTW @AdamGS and I are exploring other improvements based on this insight: |
Contributor
|
I have queued up a sql_planner run for this PR |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
|
run benchmarks |
|
🤖 Benchmark completed (GKE) | trigger Details
Resource Usagetpch — base (merge-base)
tpch — branch
|
|
🤖 Benchmark completed (GKE) | trigger Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
|
|
🤖 Benchmark completed (GKE) | trigger Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
|
2010YOUY01
reviewed
Mar 22, 2026
|
|
||
| sqllogictest-sqlite: | ||
| name: "Run sqllogictests with the sqlite test suite" | ||
| runs-on: ${{ github.repository_owner == 'apache' && format('runs-on={0},family=m8a+m7a+c8a,cpu=48,image=ubuntu24-full-x64,extras=s3-cache,disk=large,tag=datafusion,spot=false', github.run_id) || 'ubuntu-latest' }} |
Contributor
There was a problem hiding this comment.
Not directly related, but I’m curious what this is. It looks like sponsored compute resources—perhaps we could add a reference link for maintainability.
Member
Author
There was a problem hiding this comment.
looks like sponsored compute resources
yes!
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Contributor
Wow this is a significantly planning speed improvement. 🤯 |
Contributor
|
Yeah even e2e improvements on tpc-ds / tpch (probably clickbench as well but it is noisier) |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 23, 2026
…#21092) ## Which issue does this PR close? - Closes #. ## Rationale for this change This is a follow up to #21084, where @blaginin realized that allocating `ConfigOptions` has this surprising side effect. Reading through how its used I realized that on mode "real" code paths (and in many tests), DataFusion ends up allocating the default values of `SimplifyContext` just to immediately drop them and override them with pre-existing clone-able data. ## What changes are included in this PR? Adds a new type `SimplifyContextBuilder` and `SimplifyContext::builder` ## Are these changes tested? Includes a couple of tests to make sure the builder makes sense, in addition to many existing tests. ## Are there any user-facing changes? As noted above, new type and a new function on an existing type. <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> --------- Signed-off-by: Adam Gutglick <adamgsal@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Contributor
|
I think we should consider backporting this to 53 maybe even. |
6 tasks
blaginin
added a commit
to blaginin/datafusion
that referenced
this pull request
Mar 24, 2026
…1084) This makes sqlite extended finish in 5 minutes: https://github.com/apache/datafusion/actions/runs/23362665318/job/67969547959 Currently on main it takes 20 minutes (and a month ago it would take two hours 🤯) --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> (cherry picked from commit 448a395)
Member
Author
|
sure! #21137 |
de-bgunter
pushed a commit
to de-bgunter/datafusion
that referenced
this pull request
Mar 24, 2026
…1084) This makes sqlite extended finish in 5 minutes: https://github.com/apache/datafusion/actions/runs/23362665318/job/67969547959 Currently on main it takes 20 minutes (and a month ago it would take two hours 🤯) --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
de-bgunter
pushed a commit
to de-bgunter/datafusion
that referenced
this pull request
Mar 24, 2026
…apache#21092) ## Which issue does this PR close? - Closes #. ## Rationale for this change This is a follow up to apache#21084, where @blaginin realized that allocating `ConfigOptions` has this surprising side effect. Reading through how its used I realized that on mode "real" code paths (and in many tests), DataFusion ends up allocating the default values of `SimplifyContext` just to immediately drop them and override them with pre-existing clone-able data. ## What changes are included in this PR? Adds a new type `SimplifyContextBuilder` and `SimplifyContext::builder` ## Are these changes tested? Includes a couple of tests to make sure the builder makes sense, in addition to many existing tests. ## Are there any user-facing changes? As noted above, new type and a new function on an existing type. <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> --------- Signed-off-by: Adam Gutglick <adamgsal@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
alamb
added a commit
that referenced
this pull request
Mar 25, 2026
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 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.
This makes sqlite extended finish in 5 minutes: https://github.com/apache/datafusion/actions/runs/23362665318/job/67969547959
Currently on main it takes 20 minutes (and a month ago it would take two hours 🤯)