You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "friendlymatthew (via GitHub)" <gi...@apache.org> on 2025/12/01 12:07:31 UTC
[PR] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
friendlymatthew opened a new pull request, #8937:
URL: https://github.com/apache/arrow-rs/pull/8937
# Which issue does this PR close?
- Closes https://github.com/apache/arrow-rs/issues/8936
# Rationale for this change
This PR introduces a custom PartialEq impl for `UnionFields` that uses set semantics when determining equality
Currently, `UnionFields` derives PartailEq, but its internal representation is a list of `(id, data type)` tuples. As a result, equality is order-dependent, even though `UnionFields` is conceptually a set of unique `(id, data type)` pairs
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2627723416
##########
arrow-schema/src/fields.rs:
##########
@@ -435,6 +435,11 @@ impl UnionFields {
}
}
}
+
+ // sort fields by type id to ensure deterministic ordering and consistent equality comparisons
+ out.sort_unstable_by_key(|&(i, _)| i);
+
+ return Ok(Self(out.into()));
Review Comment:
> https://github.com/apache/arrow-rs/pull/8937#discussion_r2624620897
It seems sorting the UnionFields upon construction lets everything else fall into place
Note we can now `derive(PartialEq, Hash)`
cc @alamb @tobixdev
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#issuecomment-3647773059
> Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look
Hi @alamb, apologies for the delay. Work was a bit busy this week, but I have some bandwidth to get this over the line now
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2624619195
##########
arrow-schema/src/fields.rs:
##########
@@ -345,6 +345,21 @@ impl std::ops::Index<usize> for UnionFields {
}
}
+impl PartialEq for UnionFields {
+ fn eq(&self, other: &Self) -> bool {
+ self.len() == other.len() && self.iter().all(|a| other.iter().any(|b| a == b))
Review Comment:
Can you please add a comment that says this is (and should remain) consistent with the definition in DataType::Union?
##########
arrow-schema/src/fields.rs:
##########
@@ -345,6 +345,21 @@ impl std::ops::Index<usize> for UnionFields {
}
}
+impl PartialEq for UnionFields {
+ fn eq(&self, other: &Self) -> bool {
+ self.len() == other.len() && self.iter().all(|a| other.iter().any(|b| a == b))
+ }
+}
+
+impl Hash for UnionFields {
+ fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+ let mut v = self.0.iter().collect::<Vec<_>>();
Review Comment:
Another alternative might be to always sort the UnionFields on construction 🤔
##########
arrow-schema/src/fields.rs:
##########
@@ -345,6 +345,21 @@ impl std::ops::Index<usize> for UnionFields {
}
}
+impl PartialEq for UnionFields {
+ fn eq(&self, other: &Self) -> bool {
+ self.len() == other.len() && self.iter().all(|a| other.iter().any(|b| a == b))
+ }
+}
+
+impl Hash for UnionFields {
+ fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+ let mut v = self.0.iter().collect::<Vec<_>>();
Review Comment:
Allocating (the Vec) during a hash function is probably a pretty poor choice -- can we please try and find some different implementation that doesn't require a hash? For example a constant or hash the data types of the lowest and highest type_id?
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2627741033
##########
arrow-schema/src/fields.rs:
##########
@@ -329,6 +329,28 @@ impl std::fmt::Debug for UnionFields {
}
}
+impl PartialEq for UnionFields {
+ fn eq(&self, other: &Self) -> bool {
+ self.len() == other.len()
+ && self.iter().all(|a| {
+ other.iter().any(|b| {
+ a.0 == b.0
+ && a.1.is_nullable() == b.1.is_nullable()
+ && a.1.data_type().equals_datatype(b.1.data_type())
Review Comment:
Since https://github.com/apache/arrow-rs/blob/08dcc0b8c552e81d612b53bc0ab2a0007bfe20a1/arrow-schema/src/datatype.rs#L676-L688 explicitly avoids the metadata eq check, I will elect to do the same here
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2598792020
##########
arrow-schema/src/fields.rs:
##########
@@ -329,6 +329,28 @@ impl std::fmt::Debug for UnionFields {
}
}
+impl PartialEq for UnionFields {
Review Comment:
Hi, I think this PR is focused on getting a correct implementation in place
If this equality check ever shows up as the bottleneck in a profile, I'd be totally for a follow-up perf PR!
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#issuecomment-4259963671
> this PR appears now to have no changes 🤔
>
> <img alt="Screenshot 2026-04-06 at 3 53 17 PM" width="983" height="312" src="https://private-user-images.githubusercontent.com/490673/574318246-6e96b0d8-5eb7-4971-8264-36bf70c83de6.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NzYzNDE5NDYsIm5iZiI6MTc3NjM0MTY0NiwicGF0aCI6Ii80OTA2NzMvNTc0MzE4MjQ2LTZlOTZiMGQ4LTVlYjctNDk3MS04MjY0LTM2YmY3MGM4M2RlNi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjYwNDE2JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI2MDQxNlQxMjE0MDZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1mMmFhZjMyMWZmNWQ1YzYxOTU4ZjdlOGIwZTU1MDg0ODQ0MTEzYmVmZGRkODhlYzMwZWQwYzI5OTM1YmMwN2M2JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZyZXNwb25zZS1jb250ZW50LXR5cGU9aW1hZ2UlMkZwbmcifQ.dKSNOJLrz1TxnyraMrjtolOUpLq6sTNaKnEd4zgewVY">
Weird! I'll fiddle with this and ping you when it is ready
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2577663763
##########
arrow-schema/src/fields.rs:
##########
@@ -329,6 +329,28 @@ impl std::fmt::Debug for UnionFields {
}
}
+impl PartialEq for UnionFields {
Review Comment:
I think this is a possible way of getting the desired behavior. We do have some mildly large unions (10-ish currently, but maybe more in the future) and I am a bit anxious on how the quadratic equality check will perform in practice, as the equality of a data type is often used when comparing fields, schemas, and logical plans.
Another approach would be sorting the slice when constructing the instance. Maybe this would be an equally simple change with better performance as this already a private field.
Note that I have no idea how this will actually perform as I have no benchmarks. Just my 2 cents.
Otherwise, this looks like a good change!
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#issuecomment-3661235195
> > If we are going to do this, I think we should clearly document somewhere what "equal" means for `DataType::Union` and `UnionArray` and make sure the crate is consistent with this definition
>
> I agree that we should document what "equal" means for `DataType::Union` and `UnionArray`.
>
> But isn't `DataType::equals_datatype` already the authoritative definition of equality for `DataType::Union`?
>
> The specific match arm for unions is here:
>
> https://github.com/apache/arrow-rs/blob/08dcc0b8c552e81d612b53bc0ab2a0007bfe20a1/arrow-schema/src/datatype.rs#L676-L688
curious to hear your thoughts @alamb
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2599488709
##########
arrow-schema/src/fields.rs:
##########
@@ -329,6 +329,28 @@ impl std::fmt::Debug for UnionFields {
}
}
+impl PartialEq for UnionFields {
Review Comment:
maybe we can file a ticket
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2627723416
##########
arrow-schema/src/fields.rs:
##########
@@ -435,6 +435,11 @@ impl UnionFields {
}
}
}
+
+ // sort fields by type id to ensure deterministic ordering and consistent equality comparisons
+ out.sort_unstable_by_key(|&(i, _)| i);
+
+ return Ok(Self(out.into()));
Review Comment:
> https://github.com/apache/arrow-rs/pull/8937#discussion_r2624620897
It seems sorting the UnionFields upon construction lets everything else fall into place
Note we can now `derive(PartialEq, Hash)`
cc @alamb @tobixdev
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#issuecomment-3596272266
cc @alamb this should be fairly quick to review!
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#issuecomment-5336615535
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#issuecomment-3638176313
Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2599487708
##########
arrow-schema/src/fields.rs:
##########
@@ -329,6 +329,28 @@ impl std::fmt::Debug for UnionFields {
}
}
+impl PartialEq for UnionFields {
+ fn eq(&self, other: &Self) -> bool {
+ self.len() == other.len()
+ && self.iter().all(|a| {
+ other.iter().any(|b| {
+ a.0 == b.0
+ && a.1.is_nullable() == b.1.is_nullable()
+ && a.1.data_type().equals_datatype(b.1.data_type())
Review Comment:
How about the metadata?
(it might be more consistent to compare `a.1 == b.1` )
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2627741033
##########
arrow-schema/src/fields.rs:
##########
@@ -329,6 +329,28 @@ impl std::fmt::Debug for UnionFields {
}
}
+impl PartialEq for UnionFields {
+ fn eq(&self, other: &Self) -> bool {
+ self.len() == other.len()
+ && self.iter().all(|a| {
+ other.iter().any(|b| {
+ a.0 == b.0
+ && a.1.is_nullable() == b.1.is_nullable()
+ && a.1.data_type().equals_datatype(b.1.data_type())
Review Comment:
Since `DataType::equals_datatype` https://github.com/apache/arrow-rs/blob/08dcc0b8c552e81d612b53bc0ab2a0007bfe20a1/arrow-schema/src/datatype.rs#L676-L688 explicitly avoids the metadata eq check, I will elect to do the same here
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #8937: Fix `PartialEq` behavior for `UnionFields`
URL: https://github.com/apache/arrow-rs/pull/8937
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#issuecomment-4194643256
this PR appears now to have no changes 🤔
<img width="983" height="312" alt="Screenshot 2026-04-06 at 3 53 17 PM" src="https://github.com/user-attachments/assets/6e96b0d8-5eb7-4971-8264-36bf70c83de6" />
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2577663763
##########
arrow-schema/src/fields.rs:
##########
@@ -329,6 +329,28 @@ impl std::fmt::Debug for UnionFields {
}
}
+impl PartialEq for UnionFields {
Review Comment:
I think this is a possible way of getting the desired behavior. In our project, we do have some mildly large unions (10-ish currently, but maybe more in the future) that occur very frequently and I am a bit anxious on how the quadratic equality check will perform in practice, as the equality of a data type is often used when comparing fields, schemas, and logical plans.
Another approach would be sorting the slice when constructing the instance. Maybe this would be an equally simple change with better performance as this already a private field.
Note that I have no idea how this will actually perform as I have no benchmarks. Just my 2 cents.
Otherwise, this looks like a good change!
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#discussion_r2627752757
##########
arrow-schema/src/fields.rs:
##########
@@ -329,6 +329,28 @@ impl std::fmt::Debug for UnionFields {
}
}
+impl PartialEq for UnionFields {
+ fn eq(&self, other: &Self) -> bool {
+ self.len() == other.len()
+ && self.iter().all(|a| {
+ other.iter().any(|b| {
+ a.0 == b.0
+ && a.1.is_nullable() == b.1.is_nullable()
+ && a.1.data_type().equals_datatype(b.1.data_type())
Review Comment:
Actually, I take that back. It seems `DataType` impls `PartialEq` as well. I'll choose to do the same
--
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] Fix `PartialEq` behavior for `UnionFields` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #8937:
URL: https://github.com/apache/arrow-rs/pull/8937#issuecomment-3647752009
> If we are going to do this, I think we should clearly document somewhere what "equal" means for `DataType::Union` and `UnionArray` and make sure the crate is consistent with this definition
I agree that we should document what "equal" means for `DataType::Union` and `UnionArray`.
But isn't `DataType::equals_datatype` already the authoritative definition of equality for `DataType::Union`?
The specific match arm for unions is here:
https://github.com/apache/arrow-rs/blob/08dcc0b8c552e81d612b53bc0ab2a0007bfe20a1/arrow-schema/src/datatype.rs#L676-L688
--
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