remove derive(Copy) from Operator
#11132
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derive(Copy) from Operator`derive(Copy) from Operator
alamb
reviewed
Jun 26, 2024
Contributor
alamb
left a comment
There was a problem hiding this comment.
Thank you @samuelcolvin -- I think this change makes sense to me
Another option which might be less impactful and likely be more performance could be:
- Leave
#[derive(Copy)]forOperator - Model custom operators using an
Arc(soCopywould be relatively cheap)
So like
enum Operator {
...
Custom(Arc<str>)
}While making Arc Copy is not typically done, it might be ok in this case to reduce API churn 🤔
What do you think?
cc @jayzhan211
Contributor
Author
|
This suggests it's not safe to implement Perhaps I'm missing something? |
alamb
approved these changes
Jun 26, 2024
Contributor
alamb
left a comment
There was a problem hiding this comment.
Makes sense to me - I don't see any other way around this change
Thanks @samuelcolvin
This was referenced Jun 26, 2024
d544a63 to
b86655f
Compare
Contributor
|
🚀 |
Contributor
|
Thanks @samuelcolvin |
alamb
added a commit
to alamb/datafusion
that referenced
this pull request
Jul 8, 2024
This reverts commit b468ba7.
andygrove
pushed a commit
that referenced
this pull request
Jul 9, 2024
findepi
pushed a commit
to findepi/datafusion
that referenced
this pull request
Jul 16, 2024
* remove derive Copy from Operator * fix formatting and more cloning * fix remaining case * fix docs case * fix operator borrows
xinlifoobar
pushed a commit
to xinlifoobar/datafusion
that referenced
this pull request
Jul 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
This is the first step towards solving #10981 (comment) - providing a general way to support more operators without having them all added to datafusion core.
The idea is that this would allow
Operatorto include another member of the union that in turn could includingCustom(String), but also potentially a function that defines extra operators' signatures, without having to hard code them intodatafusion/expr/src/type_coercion/binary.rs::signature.I'm creating this as a standalone PR, so:
To see if there's willingness to accept this change before I go forwardThe rest of the work is ready for review - Custom operator support #11137What changes are included in this PR?
Copytrait is no longer derived forOperator(this meansOperatorbetter matches most other related logical plan structs)Operatorare updated to minimise changes while avoid clones where possibleAre these changes tested?
Yes, existing tests (should) pass.
Are there any user-facing changes?
Some public function signatures might have changed, if you're willing to consider this change, let me know if you want me to revert some of those public signature changes.