feat: impl PartialEq + Eq for GetOptions & PutPayload
#7152
Conversation
This is useful for testing and pretty simple.
| @@ -19,7 +19,7 @@ use bytes::Bytes; | |||
| use std::sync::Arc; | |||
|
|
|||
| /// A cheaply cloneable, ordered collection of [`Bytes`] | |||
| #[derive(Debug, Clone)] | |||
| #[derive(Debug, Clone, PartialEq, Eq)] | |||
There was a problem hiding this comment.
I wonder if people might expect this to be agnostic to chunking?
There was a problem hiding this comment.
Good question. I think NOT equalizing chunking might be the more correct Eq implementation, because if you want to test optimizations / implementation details, this is important. I think we should document it though.
| @@ -916,7 +916,7 @@ pub struct ObjectMeta { | |||
| } | |||
|
|
|||
| /// Options for a get request, such as range | |||
| #[derive(Debug, Default, Clone)] | |||
| #[derive(Debug, Default, Clone, PartialEq, Eq)] | |||
| pub struct GetOptions { | |||
There was a problem hiding this comment.
Will this work if we add a dyn Any context/extensions field as being discussed in #7135
There was a problem hiding this comment.
Should we require extensions to implement not only Any but also Eq and PartialEq w/ their own type? That would require some additional bookkeeping code withing AnyMap to store the vtable entries though, or don't use Any at all but trait Extension: Eq + std::fmt::Debug or something like that. The more I think about it, the more convinced I am that a proper new trait would be better.
There was a problem hiding this comment.
Eq and PartialEq aren't dyn-compatible because they take Self as a type parameter: https://doc.rust-lang.org/error_codes/E0038.html#trait-uses-self-as-a-type-parameter-in-the-supertrait-listing
We could work around that with custom PartialEq and Eq implementations that ignore the extensions field discussed in https://github.com/apache/arrow-rs/issues/7155 (opened in favor of #7135). Given that the extensions are meant to be used by custom ObjectStore implementations/wrappers and ignored by the builtin backends this shouldn't be a problem, right?
There was a problem hiding this comment.
Yes it's not dyn-safe, but you still can implement this via a trait and the right signature, see #7160 (comment)
|
What are we thinking about the current state of this PR? Are we happy with it? |
|
Thank you for this PR. We are in the process of moving the object_store code to its own repository. Would it be possible for you to create a PR in that repository instead?
(we will handle moving all existing issues to the new repository) |
Which issue does this PR close?
-
Rationale for this change
This is useful for testing and pretty simple.
What changes are included in this PR?
Added some derives.
Are there any user-facing changes?
Traits are now implemented.