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

remove derive(Copy) from Operator #11132

Merged
merged 5 commits into from
Jun 27, 2024
Merged

Conversation

samuelcolvin
Copy link
Contributor

@samuelcolvin samuelcolvin commented Jun 26, 2024

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 Operator to include another member of the union that in turn could including Custom(String), but also potentially a function that defines extra operators' signatures, without having to hard code them into datafusion/expr/src/type_coercion/binary.rs::signature.

I'm creating this as a standalone PR, so:

  1. It doesn't pollute other changes
  2. To see if there's willingness to accept this change before I go forward The rest of the work is ready for review - Custom operator support #11137

What changes are included in this PR?

  • The Copy trait is no longer derived for Operator (this means Operator better matches most other related logical plan structs)
  • functions using Operator are updated to minimise changes while avoid clones where possible

Are 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.

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate labels Jun 26, 2024
@samuelcolvin samuelcolvin changed the title remove derive(Copy) from Operator` remove derive(Copy) from Operator Jun 26, 2024
@alamb alamb added the api change Changes the API exposed to users of the crate label Jun 26, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Leave #[derive(Copy)] for Operator
  2. Model custom operators using an Arc (so Copy would 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

@samuelcolvin
Copy link
Contributor Author

This suggests it's not safe to implement Copy on an Arc.

Perhaps I'm missing something?

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me - I don't see any other way around this change

Thanks @samuelcolvin

@alamb
Copy link
Contributor

alamb commented Jun 27, 2024

🚀

@alamb alamb merged commit b468ba7 into apache:main Jun 27, 2024
24 checks passed
@alamb
Copy link
Contributor

alamb commented Jun 27, 2024

Thanks @samuelcolvin

@alamb alamb mentioned this pull request Jul 8, 2024
alamb added a commit to alamb/datafusion that referenced this pull request Jul 8, 2024
andygrove pushed a commit that referenced this pull request Jul 9, 2024
@samuelcolvin samuelcolvin deleted the operator-clone branch July 9, 2024 08:01
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
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
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
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 physical-expr Physical Expressions substrait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants