Uh oh!
There was an error while loading. Please reload this page.
Add a builder to SimplifyContext to avoid allocating default values - #21092
Conversation
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
Dandandan
commented
Mar 21, 2026
Run benchmark sql_planner |
adriangbot
commented
Mar 21, 2026
🤖 Criterion benchmark running (GKE) | trigger |
adriangbot
commented
Mar 21, 2026
Benchmark for this request failed. Last 20 lines of output: Click to expand |
AdamGS
commented
Mar 21, 2026
@Dandandan should I rebase this on top of #21084? |
alamb
left a comment
There was a problem hiding this comment.
I think this is an API improvement even if it doesn't measurably improve the performance -- thank you @AdamGS
One thing I thought of that might be less invasive (as in require less code changes) would be to use a single global lazy lock for default config options. Something like
/// Global default configuration optionsstaticDEFAULT_CONFIG:LazyLock<Arc<ConfigOptions>> = LazyLock::new(|| Arc::new(ConfigOptions::new()));implConfigOptions{/// Creates a new [`ConfigOptions`] with default valuespubfnnew() -> Self{Self::default()}/// Return a reference to the default configuration optionspubfndefault_arc() -> Arc<Self>{Arc::clone(&DEFAULT_CONFIG)}Let me see if I can make a PR that does this
| let state = self.state.read(); | ||
| let context = SimplifyContext::default() | ||
| let context = SimplifyContext::builder() | ||
| .with_schema(Arc::clone(prepared.plan.schema())) |
There was a problem hiding this comment.
I recommend we also deprecate SimplifyContext::with... methods (can do it as a follow on PR) and direct people to use the builder in their own code.
There was a problem hiding this comment.
That makes perfect sense, I'll add it in this one
alamb
commented
Mar 21, 2026
I made I have queued up some benchmark runs (sql_planner) for these branches -- let's see what they say (should be done in an hour or two) |
What should the edit: FWIW - I'll happily backport this to 53.1.0 if necessary |
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
0de841f to
8d0c0d5Comparealamb
commented
Mar 21, 2026
54 for now (and we can change it to 53 if we choose to backport) |
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
alamb-ghbot
commented
Mar 21, 2026
🤖 |
alamb-ghbot
commented
Mar 21, 2026
🤖: Benchmark completed Details |
Dandandan
commented
Mar 22, 2026
Woah, is this right? |
Dandandan
commented
Mar 22, 2026
run benchmarks |
adriangbot
commented
Mar 22, 2026
adriangbot
commented
Mar 22, 2026
adriangbot
commented
Mar 22, 2026
adriangbot
commented
Mar 22, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usagetpch — base (merge-base)
tpch — branch
|
adriangbot
commented
Mar 22, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usagetpcds — base (merge-base)
tpcds — branch
|
adriangbot
commented
Mar 22, 2026
🤖 Benchmark completed (GKE) | trigger DetailsResource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
|
alamb
commented
Mar 22, 2026
I bet it is less of a win after #21084 is merged. I'll update and rerun |
alamb-ghbot
commented
Mar 22, 2026
🤖 |
alamb-ghbot
commented
Mar 22, 2026
🤖: Benchmark completed Details |
alamb
commented
Mar 22, 2026
Seems like still a small win |
alamb-ghbot
commented
Mar 22, 2026
🤖 |
alamb-ghbot
commented
Mar 22, 2026
🤖: Benchmark completed Details |
alamb
commented
Mar 23, 2026
This still shows a nice improvement. I think we should proceed with this PR and we can discuss also doing |
blaginin
left a comment
There was a problem hiding this comment.
agreed! it has performance impact and it also makes API much nicer and more consistent
Uh oh!
There was an error while loading. Please reload this page.
…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
commented
Mar 24, 2026
🚀 |
Which issue does this PR close?
Rationale for this change
This is a follow up to #21084, where @blaginin realized that allocating
ConfigOptionshas 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 ofSimplifyContextjust to immediately drop them and override them with pre-existing clone-able data.What changes are included in this PR?
Adds a new type
SimplifyContextBuilderandSimplifyContext::builderAre 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.