You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "crepererum (via GitHub)" <gi...@apache.org> on 2025/02/18 15:21:31 UTC
[PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
crepererum opened a new pull request, #7152:
URL: https://github.com/apache/arrow-rs/pull/7152
# 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "waynr (via GitHub)" <gi...@apache.org>.
waynr commented on code in PR #7152:
URL: https://github.com/apache/arrow-rs/pull/7152#discussion_r1962352629
##########
object_store/src/lib.rs:
##########
@@ -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 {
Review 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 https://github.com/apache/arrow-rs/issues/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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #7152:
URL: https://github.com/apache/arrow-rs/pull/7152#discussion_r1961141517
##########
object_store/src/lib.rs:
##########
@@ -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 {
Review Comment:
Will this work if we add a dyn Any context/extensions field as being discussed in #7135
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "waynr (via GitHub)" <gi...@apache.org>.
waynr commented on code in PR #7152:
URL: https://github.com/apache/arrow-rs/pull/7152#discussion_r1962352629
##########
object_store/src/lib.rs:
##########
@@ -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 {
Review 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 https://github.com/apache/arrow-rs/issues/7135). Given that the extensions are meant to be used by customer `ObjectStore` implementations/wrappers and ignored by the builtin backends this shouldn't be a problem, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #7152:
URL: https://github.com/apache/arrow-rs/pull/7152#discussion_r1960134182
##########
object_store/src/payload.rs:
##########
@@ -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)]
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7152:
URL: https://github.com/apache/arrow-rs/pull/7152#issuecomment-2706293318
What are we thinking about the current state of this PR? Are we happy with it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #7152:
URL: https://github.com/apache/arrow-rs/pull/7152#discussion_r1960128994
##########
object_store/src/payload.rs:
##########
@@ -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)]
Review Comment:
I wonder if people might expect this to be agnostic to chunking?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #7152:
URL: https://github.com/apache/arrow-rs/pull/7152#discussion_r1961634737
##########
object_store/src/lib.rs:
##########
@@ -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 {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #7152:
URL: https://github.com/apache/arrow-rs/pull/7152#discussion_r1963288580
##########
object_store/src/lib.rs:
##########
@@ -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 {
Review Comment:
Yes it's not dyn-safe, but you still can implement this via a trait and the right signature, see https://github.com/apache/arrow-rs/pull/7160#issuecomment-2671069353
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #7152: feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload`
URL: https://github.com/apache/arrow-rs/pull/7152
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7152:
URL: https://github.com/apache/arrow-rs/pull/7152#issuecomment-2749216116
Please open a new PR in the new repository:
- https://github.com/apache/arrow-rs-object-store
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: impl `PartialEq + Eq` for `GetOptions` & `PutPayload` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7152:
URL: https://github.com/apache/arrow-rs/pull/7152#issuecomment-2741515119
Thank you for this PR. We are in the process of moving the object_store code to its [own repository](https://github.com/apache/arrow-rs-object-store). Would it be possible for you to create a PR in that repository instead?
- See details on https://github.com/apache/arrow-rs-object-store/issues/2
- Backstory on https://github.com/apache/arrow-rs/issues/6183
(we will handle moving all existing issues to the new repository)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org