Skip to content

Conversation

@sadboy
Copy link
Contributor

@sadboy sadboy commented Dec 20, 2023

Which issue does this PR close?

N/A. Addresses a TODO item in tree_node/expr.rs

Rationale for this change

Inspired by #7942: currently, the Expr transformer's transform_box and transform_vec methods allocate a new Box and Vec, respectively, with potentially harmful performance implications. We can use std::mem::replace to avoid this allocation by reusing the existing Box/Vec.

What changes are included in this PR?

Switch to in-place mutations in the transform_box and transform_vec methods on Exprs.

Are these changes tested?

Yes, existing tests.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Dec 20, 2023
@sadboy
Copy link
Contributor Author

sadboy commented Dec 20, 2023

This change doesn't do anything -- I verified both transform_box and transform_vec are already in-place mutations with no unnecessary allocations as written. We can probably just update the comments with a note.

Closing this one.

@sadboy sadboy closed this Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant