Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace logical plan from Arc<T> to Box<T> #9763

Closed
wants to merge 7 commits into from

Conversation

jayzhan211
Copy link
Contributor

@jayzhan211 jayzhan211 commented Mar 24, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

#9637
I think it is easier to avoid clone after the plan are all boxed instead of arced

What changes are included in this PR?

  1. Change Arc to Box
  2. union is changed to Vec instead of Vec<Box>

Are these changes tested?

Are there any user-facing changes?

Screenshot 2024-03-24 at 12 05 54 PM

Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate substrait labels Mar 24, 2024
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
@jayzhan211 jayzhan211 added the api change Changes the API exposed to users of the crate label Mar 24, 2024
@jayzhan211 jayzhan211 changed the title DRAFT: Replace logical plan from Arc<T> to Box<T> Replace logical plan from Arc<T> to Box<T> Mar 24, 2024
@jayzhan211 jayzhan211 marked this pull request as ready for review March 24, 2024 12:22
@jayzhan211 jayzhan211 marked this pull request as draft March 24, 2024 12:25
@jayzhan211
Copy link
Contributor Author

It is part of #9768 , if #9768 is too complex to review, we can merge this first.

@alamb
Copy link
Contributor

alamb commented Mar 24, 2024

Thanks @jayzhan211 -- I only just saw that you were working on this -- I agree this would be a good change. However, I think it will be a substantial performance regression until we also avoid so many logical plan clone's in the optimizer

Copy link

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale PR has not had any activity for some time label May 24, 2024
@jayzhan211 jayzhan211 closed this May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change Changes the API exposed to users of the crate core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner Stale PR has not had any activity for some time substrait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants