Exclude dict_id and dict_is_ordered from equality comparison of Field
#1647
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1647 +/- ##
==========================================
+ Coverage 83.02% 83.09% +0.06%
==========================================
Files 193 193
Lines 55612 55894 +282
==========================================
+ Hits 46174 46447 +273
- Misses 9438 9447 +9
Continue to review full report at Codecov.
|
arrow/src/datatypes/field.rs
Outdated
| @@ -27,7 +28,7 @@ use super::DataType; | |||
| /// Contains the meta-data for a single relative type. | |||
| /// | |||
| /// The `Schema` object is an ordered collection of `Field` objects. | |||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] | |||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialOrd, Ord)] | |||
There was a problem hiding this comment.
I think you need to also implement PartialOrd??
There was a problem hiding this comment.
Hmm, okay, I just considered equality comparison case, I think it is mostly common. Not sure when exact order between Field would be needed?
There was a problem hiding this comment.
I think there might be funkiness if PartialOrd and PartialEq have different ideas of equality?
There was a problem hiding this comment.
Okay, make sense. Let me update this. Thanks.
arrow/src/datatypes/field.rs
Outdated
|
|
||
| impl Ord for Field { | ||
| fn cmp(&self, other: &Self) -> Ordering { | ||
| let mut ord = self.name.cmp(other.name()); |
There was a problem hiding this comment.
You can use https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.then to make this shorter, but this is also fine
| @@ -39,6 +41,50 @@ pub struct Field { | |||
| metadata: Option<BTreeMap<String, String>>, | |||
| } | |||
|
|
|||
| impl PartialEq for Field { | |||
There was a problem hiding this comment.
We could probably do with a comment to say why we are implementing these instead of an auto-derive
| self.metadata.hash(state); | ||
| } | ||
| } | ||
|
|
|
cc @jhorstmann |
Which issue does this PR close?
Closes #1646.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?