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

Support copy, deepcopy, eq on types #99

Merged
merged 5 commits into from
Aug 4, 2023

Conversation

GodTamIt
Copy link
Contributor

@GodTamIt GodTamIt commented Jul 21, 2023

This change implements the __richcmp__, __copy__() and __deepcopy__() functions for certain types that can be copied and compared.

@cjrh
Copy link
Collaborator

cjrh commented Jul 22, 2023

Looks good. Just need python tests on the types with the new impls. Document, Facet and SearchResult right?

@GodTamIt GodTamIt changed the title Support copy and deepcopy on types Support copy, deepcopy, eq on types Jul 22, 2023
@GodTamIt GodTamIt requested a review from adamreichold July 22, 2023 20:34
@GodTamIt
Copy link
Contributor Author

GodTamIt commented Jul 22, 2023

I just realized that marking classes as frozen won't work with pickling 🙁 (__setstate__ needs the class to be mutable)

@adamreichold
Copy link
Collaborator

I just realized that marking classes as frozen won't work with pickling slightly_frowning_face (__setstate__ needs the class to be mutable)

Since immutability is highly desirable for both performance and correctness, especially in a language like Python built around pervasive shared mutability, I would recommend not dropping it easily to implement the Pickle protocol. (PyO3 is even planning to make frozen classes the default eventually due to this.)

Maybe we can use #96 to take a step back and reconsider why Pickle support was requested? Is full integration with the Pickle protocol necessary or only (de-)serialization from PyBytes which could also be implemented using methods returning newly created immutable instances?

@GodTamIt
Copy link
Contributor Author

Since immutability is highly desirable for both performance and correctness, especially in a language like Python built around pervasive shared mutability, I would recommend not dropping it easily to implement the Pickle protocol. (PyO3 is even planning to make frozen classes the default eventually due to this.)

I was able to accomplish pickling with __reduce__ and/or __getnewargs__ and pythonize in the change. No mutability required for those.

This change is okay regardless.

@GodTamIt GodTamIt requested a review from cjrh August 1, 2023 20:46
@GodTamIt GodTamIt force-pushed the godtamit-copy-upstream branch from 2a2fca0 to 295745e Compare August 3, 2023 14:00
@GodTamIt GodTamIt requested a review from cjrh August 3, 2023 14:03
Copy link
Collaborator

@cjrh cjrh left a comment

Choose a reason for hiding this comment

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

I'm good to move forward, but I'd like to get feedback from @adamreichold too before I merge.

@cjrh cjrh merged commit 8b33e00 into quickwit-oss:master Aug 4, 2023
@GodTamIt GodTamIt deleted the godtamit-copy-upstream branch August 4, 2023 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants