Fix PartialEq behavior for UnionFields

[friendlymatthew]

Which issue does this PR close?

• Closes Incorrect PartialEq behavior for UnionFields #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

[friendlymatthew]

cc @alamb this should be fairly quick to review!

[tobixdev]

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!

[friendlymatthew]

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!

[alamb]

maybe we can file a ticket

[alamb]

How about the metadata?

(it might be more consistent to compare a.1 == b.1 )

[friendlymatthew]

Since DataType::equals_datatype

arrow-rs/arrow-schema/src/datatype.rs

Lines 676 to 688 in 08dcc0b

	DataType::Union(a_union_fields, a_union_mode),
	DataType::Union(b_union_fields, b_union_mode),
	) => {
	a_union_mode == b_union_mode
	&& a_union_fields.len() == b_union_fields.len()
	&& a_union_fields.iter().all(|a| {
	b_union_fields.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())
	})
	})
	}

explicitly avoids the metadata eq check, I will elect to do the same here

[friendlymatthew]

Actually, I take that back. It seems DataType impls PartialEq as well. I'll choose to do the same

[alamb]

Thank you @friendlymatthew and @tobixdev -- I am not sure about this PR.

Specifically, I am not sure about (re)defining what equality means for Unions - I think it is important that Union equality is consistently defined across the crate

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

Can you research what the current definition UnionArray is?

[alamb]

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

[friendlymatthew]

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:

arrow-rs/arrow-schema/src/datatype.rs

Lines 676 to 688 in 08dcc0b

	DataType::Union(a_union_fields, a_union_mode),
	DataType::Union(b_union_fields, b_union_mode),
	) => {
	a_union_mode == b_union_mode
	&& a_union_fields.len() == b_union_fields.len()
	&& a_union_fields.iter().all(|a| {
	b_union_fields.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())
	})
	})
	}

[friendlymatthew]

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

[friendlymatthew]

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:

arrow-rs/arrow-schema/src/datatype.rs

Lines 676 to 688 in 08dcc0b

	DataType::Union(a_union_fields, a_union_mode),
	DataType::Union(b_union_fields, b_union_mode),
	) => {
	a_union_mode == b_union_mode
	&& a_union_fields.len() == b_union_fields.len()
	&& a_union_fields.iter().all(|a| {
	b_union_fields.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())
	})
	})
	}

curious to hear your thoughts @alamb

[alamb]

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?

[alamb]

Another alternative might be to always sort the UnionFields on construction 🤔

[alamb]

Can you please add a comment that says this is (and should remain) consistent with the definition in DataType::Union?

[Jefffrey]

If the approach is to ensure the Arc<[]> inside UnionFields is always sorted, we need to include that documentation as part of its docstring for visibility. Also, I see only try_new is modified with this new behaviour, but there's a whole lot of other APIs there which construct UnionFIelds without going through try_new() so they would need to also be updated; for example, new(), from_fields(), the FromIterator implementation, etc.