Uh oh!
There was an error while loading. Please reload this page.
Optimize planning / stop cloning Strings / Fields so much (2-3% faster planning time) - #18415
Conversation
| metadata, | ||
| .. | ||
| }) => { | ||
| let field = expr.to_field(schema).map(|(_, f)| f.as_ref().clone())?; |
There was a problem hiding this comment.
these as_ref().clone() type methods are doing a deep clone on the Fields each time even when the name is not changing
alamb
commented
Oct 31, 2025
🤖 |
| /// Returns an immutable reference of a specific `Field` instance selected using an | ||
| /// offset within the internal `fields` vector | ||
| pub fn field(&self, i: usize) -> &Field { | ||
| pub fn field(&self, i: usize) -> &FieldRef { |
There was a problem hiding this comment.
the changes in this file are the public API changes -- they return a reference to the Arc rather than what is in the Arc meaning the callsite can clone the arcs when needed
alamb
commented
Oct 31, 2025
🤖: Benchmark completed Details |
alamb
commented
Oct 31, 2025
🤖 |
alamb
commented
Nov 1, 2025
🤖: Benchmark completed Details |
074e485 to
879d1bfCompare879d1bf to
9dcfdfcCompare| let filter_df_fields = filter_df_fields | ||
| .into_iter() | ||
| .map(|(qualifier, field)| { | ||
| (qualifier.cloned(), Arc::new(field.clone())) |
There was a problem hiding this comment.
this was always a deep clone that is now remved
alamb
commented
Nov 11, 2025
🤖 |
alamb
commented
Nov 12, 2025
🤖: Benchmark completed Details |
alamb
commented
Nov 12, 2025
🤖 |
alamb
commented
Nov 12, 2025
🤖: Benchmark completed Details |
| } | ||
| impl FieldExt for Field { | ||
| fn renamed(self, new_name: &str) -> Self { |
There was a problem hiding this comment.
I added several new methods to avoid cloning when it is unnecesary
| @@ -429,38 +430,36 @@ | |||
There was a problem hiding this comment.
this is called as part of Expr::nullable and Expr::nullable so it is a very hot path on planning
alamb
commented
Nov 25, 2025
@Jefffrey do you happen to have time to review this PR? |
Uh oh!
There was an error while loading. Please reload this page.
| @@ -353,13 +353,13 @@ impl DFSchema { | |||
| /// Returns an immutable reference of a specific `Field` instance selected using an | |||
There was a problem hiding this comment.
Perhaps need to update some of these method docstrings since they assume returning a Field
| /// let renamed_field = int_field.renamed("your_int"); | ||
| /// assert_eq!(renamed_field.name(), "your_int"); | ||
| /// ``` | ||
| fn renamed(self, new_name: &str) -> Self; |
There was a problem hiding this comment.
I wonder for these new methods if there is existing documentation elsewhere we'd need to upgrade to make these new methods more visible? i.e. "use these new methods if possible", though not sure how relevant might be considering these mainly replace the old as_ref from Arc -> clone -> modify field -> wrap in new Arc loop 🤔
There was a problem hiding this comment.
it is a good idea -- I will review the existing docs and try to make it more obvious / clear how to use the new methdods
There was a problem hiding this comment.
I tried to update the documentation with some more details on how to discover the new traits
Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
…on into alamb/stop_cloning_fields
alamb
commented
Dec 1, 2025
Thanks @xudong963 -- the good news is I think there is some more significant improvement we could make (by not copying strings so much). I'll try and explore it when I have more time |
alamb
commented
Dec 1, 2025
run benchmarks sql_planner |
alamb
commented
Dec 1, 2025
🤖 Hi @alamb, thanks for the request (#18415 (comment)).
Please choose one or more of these with |
alamb
commented
Dec 1, 2025
run benchmark sql_planner |
alamb
commented
Dec 1, 2025
show benchmark queue |
alamb
commented
Dec 1, 2025
🤖 Hi @alamb, you asked to view the benchmark queue (#18415 (comment)).
|
alamb
commented
Dec 1, 2025
🤖 |
alamb
commented
Dec 1, 2025
🤖: Benchmark completed Details |
alamb
commented
Dec 1, 2025
Thanks again @xudong963 and @Jefffrey |
Uh oh!
There was an error while loading. Please reload this page.
Which issue does this PR close?
Rationale for this change
Avoid a bunch of clones / String copies during planning
What changes are included in this PR?
Change several methods on DFSchema to return
&FieldRefrather than&Fieldwhich permitsArc::clonerather than a deepFieldcloneAre these changes tested?
yes by CI
I also ran benchmarks that show a small but consistent speedup in the planning benchmarks
Are there any user-facing changes?
Yes, there are several API changes in DFSchema that now return
FieldRefrather thanFieldwhich allows usingArc::clonerather thanclone. I have updated the upgrading guide too