StructArray field names should be unique

[connortsui20]

Issue Description

We have this in our doc comment for StructArray:

vortex/vortex-array/src/arrays/struct_/array.rs

Lines 71 to 97 in a30298b

	/// ## Name uniqueness
	///
	/// It is valid for a StructArray to have multiple child columns that have the same name. In this
	/// case, any accessors that use column names will find the first column in sequence with the name.
	///
	/// ```
	/// use vortex_array::arrays::StructArray;
	/// use vortex_array::validity::Validity;
	/// use vortex_array::IntoArray;
	/// use vortex_dtype::FieldNames;
	/// use vortex_buffer::buffer;
	///
	/// // Create struct with duplicate "data" field names
	/// let struct_array = StructArray::try_new(
	/// FieldNames::from(["data", "data"]),
	/// vec![
	/// buffer![1i32, 2i32].into_array(), // first "data"
	/// buffer![3i32, 4i32].into_array(), // second "data"
	/// ],
	/// 2,
	/// Validity::NonNullable,
	/// ).unwrap();
	///
	/// // field_by_name returns the FIRST "data" field
	/// let first_data = struct_array.field_by_name("data").unwrap();
	/// assert_eq!(first_data.scalar_at(0), 1i32.into());
	/// ```

To me, this is insane behavior, and not at all expected.

I am guessing that we have this behavior because the Arrow specification does not enforce this at all.

Here are relevant links:

• arrow-rs implementation
• Apache (old?) issue: https://issues.apache.org/jira/browse/ARROW-11178
• Other Apache issue (failed to migrate?): [Rust] StructArray: handling duplicate field names apache/arrow#27083

Also, looking at other Arrow issues like apache/arrow-rs#4908, it doesn't seem like they have considered things like casting and schema evolution in the context of having duplicate field names.

Edit: Arrow-rs issue apache/arrow-rs#9205

Expected Behavior

I would expect field name uniqueness to be enforced.

I cannot think of any good reason to have duplicate names, and can think of many reasons not to have this behavior...

Actual Behavior

Field names can be duplicated (and all duplicates can have DIFFERENT TYPES). When looking up fields with a given name, we (arbitrarily) get the first one.

Reproduction Steps

This is likely one of the reasons for this fuzzer crash: #5865

Technically this is not a bug but a feature, but this is so unexpected that I would consider this similar...

I acknowledge that:

• I have searched the Vortex repository (both open and closed Discussions and Issues) and confirm this is not a duplicate of an existing issue or discussion.
• I have checked the "Preview" tab on all text fields to ensure that everything looks right, and have wrapped all configuration and code in code blocks with a group of three backticks (```) on separate lines.

[robert3005]

Checking this end up being expensive anytime you have to construct a struct array. We have expanded cast to support by name casting but the fix could be doing this with two expressions where one is ordinal and the other one is name based

[connortsui20]

I don't really see how this is more expensive than the checks we already do for StructArray? Checking this is O(n) and we already have to scan the fields:

vortex/vortex-array/src/arrays/struct_/array.rs

Line 284 in a30298b

pub fn validate(

To be honest, having 2 different expressions just to cover this (what I would consider this to be) strange behavior seems like a bandaid fix.

[robert3005]

We can add this check and see the impact. When I tried it it was noticeable ~5-10% across the board. The iteration is not the problem but building the hashset is. Enough might have changed that this is no longer a problem so we can give it a try. But essentially if you have vortex -> arrow -> vortex that is no longer a free conversion

[connortsui20]

For small numbers of fields we can simply do the O(n^2) double loop to check (and of course there exist many optimizations we can do for string uniqueness).

But I don't think performance is the main topic of discussion here. Really, it is about if we care that Arrow does things in a different way or not (where we then have to figure out if we reject the external array or try to convert it).

Personally, I think it's fine to reject people who need to have duplicate named fields because they likely have a bunch of other problems that they need to deal with first.

Also, I wouldn't really classify the vortex -> arrow -> vortex right now as free anyways:

Details

vortex/vortex-array/src/arrow/convert.rs

Lines 363 to 370 in a30298b

	impl FromArrowArray<&ArrowStructArray> for ArrayRef {
	fn from_arrow(value: &ArrowStructArray, nullable: bool) -> Self {
	StructArray::try_new(
	value.column_names().iter().copied().collect(),
	value
	.columns()
	.iter()
	.zip(value.fields())

vortex/vortex-array/src/arrow/compute/to_arrow/canonical.rs

Lines 435 to 442 in a30298b

	fn to_arrow_struct(
	array: StructArray,
	fields: &[FieldRef],
	to_preferred: bool,
	) -> VortexResult<ArrowArrayRef> {
	if array.fields().len() != fields.len() {
	vortex_bail!(
	"StructArray has {} fields, but target Arrow type has {} fields",

[AdamGS]

some system (like datafusion, and definitely parts of arrow-rs) use names only as a cosmetic part of the data, often using indexes to reference specific fields/columns (see RecordBatch::project) which is strictly better performance wise. The codebase is already full of checks and validations that end up getting called repeatedly and which we keep building workarounds for ad nauseam.

Docs are not a silver bullet, but as long as the behavior is consistent and well documented users will be just fine.