perf: Improve planning time w/ global default ConfigOptions#21093
perf: Improve planning time w/ global default ConfigOptions#21093alamb wants to merge 4 commits intoapache:mainfrom
ConfigOptions#21093Conversation
|
I am running some benchmarks on this branch as well -- results will be posted in a few hours |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
run benchmarks |
1 similar comment
|
run benchmarks |
|
🤖 Benchmark completed (GKE) | trigger Details
Resource Usagetpch — base (merge-base)
tpch — branch
|
|
🤖 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
|
|
🤖 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
|
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
@AdamGS I wonder what you think of this PR compared to the builder you propose in I think they both accomplish the same basic thing, but this one doesn't require any downstream code changes to get the benefit |
ConfigOptions
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
@alamb My only concern with this is that it might be misused somewhere by someone ignoring a provided config from the session, but that's mostly a hypothetical and this is nicer for tests and has basically no API churn. |
|
One potential option is to backport this one to 53 and 52 (as it has no churn), and leave this PR on main |
|
Seems like any non-empty permutation here is an improvement, they all look good to me |
Which issue does this PR close?
SimplifyContextto avoid allocating default values #21092 from @AdamGSRationale for this change
While reviewing #21092 I realized that we could potentially keep the existing APIs but stop copying the default object if we used a global default singleton
What changes are included in this PR?
Are these changes tested?
Functioanlly they are tested by CI
I will also run planning benchmarks
Are there any user-facing changes?