-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Derive Clone
for more ExecutionPlans
#13203
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should make fields public instead.
See #13202 (comment)
@@ -293,7 +293,7 @@ impl JoinLeftData { | |||
/// │ "dimension" │ │ "fact" │ | |||
/// └───────────────┘ └───────────────┘ | |||
/// ``` | |||
#[derive(Debug)] | |||
#[derive(Debug)] // note not Clone because of the OnceAsync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be amazing to clarify what is OnceAsync and why clone is not fit for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my attempt: #13223
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, in that case we can highlight it in doc? OnceAsync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea -- I tried to clarify in b5ffef0
I am not sure making the fields Even if the fields are I don't have a strong preference one way or the other over pub fields (or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @alamb 💪
🚀 |
Which issue does this PR close?
Closes # #13202
Rationale for this change
See #13202
What changes are included in this PR?
Clone
for other ExecutionPlansNote there are some ExecutionPlans that are not clonable (like
HashJoinExec
) because they have some shared state (a once_fut used to coordinate amongst threads). It was hard to see how to add Clone to those so I left it alone but left a comment for future readersAre these changes tested?
By existing CI tests
Are there any user-facing changes?