This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-11840-adcec44f4c093f24ef75d424691639f000a34577 in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
commit b08665f6aa0b62ac85f44bf8a08948f275320d7e Author: Bharadwaj Pendyala <[email protected]> AuthorDate: Wed Oct 28 15:12:24 2026 +0000 refactor: check union type ids directly in `UnionFields::try_merge` (#11840) # Which issue does this PR close? - Closes #11575. # Rationale for this change `try_merge` couldn't actually produce duplicate type ids. If `other` brought a new field whose id was already taken, the trailing `try_new` pass rejected it. The TODOs were stale, and the error was a bare `duplicate type id: 0` that didn't say which union members clashed. # What changes are included in this PR? - When an incoming field is new and its type id is already in `output`, `try_merge` now returns a `SchemaError` naming both fields: `Fail to merge schema field 'c12' because type id 0 is already used by field 'c11'`. - The merged `UnionFields` is built from `output` directly, without the second pass through `try_new`. Both inputs are already valid, and the new check is the only way a duplicate could get in. - Both TODOs are removed. # Are these changes tested? Yes. The union block in `test_schema_merge` now merges two `c1` unions that put different fields under type id 0 and checks the new message. Without the `fields.rs` change it fails with `Invalid argument error: duplicate type id: 0`. # Are there any significant user-facing changes? The error variant for this case changes. `Field::try_merge` and `Schema::try_merge` now return `ArrowError::SchemaError` instead of `ArrowError::InvalidArgumentError`. `SchemaError` is the variant the type id mismatch check right above it already uses. The code, the test and this description were written with AI assistance (Claude). --- arrow-schema/src/fields.rs | 14 +++++++++----- arrow-schema/src/schema.rs | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/arrow-schema/src/fields.rs b/arrow-schema/src/fields.rs index c68f179fcb..3c698c8939 100644 --- a/arrow-schema/src/fields.rs +++ b/arrow-schema/src/fields.rs @@ -624,7 +624,6 @@ impl UnionFields { /// /// See [`Field::try_merge`] pub(crate) fn try_merge(&mut self, other: &Self) -> Result<(), ArrowError> { - // TODO: This currently may produce duplicate type IDs (#3982) let mut output: Vec<_> = self.iter().map(|(id, f)| (id, f.clone())).collect(); for (field_type_id, from_field) in other.iter() { let mut is_new_field = true; @@ -647,13 +646,18 @@ impl UnionFields { } if is_new_field { + if let Some((_, self_field)) = output.iter().find(|(id, _)| *id == field_type_id) { + return Err(ArrowError::SchemaError(format!( + "Fail to merge schema field '{}' because type id {} is already used by field '{}'", + from_field.name(), + field_type_id, + self_field.name() + ))); + } output.push((field_type_id, from_field.clone())) } } - // TODO: see above comment; can we remove the need to go through try_new - // and just validate in this function ourselves? - let (type_ids, fields): (Vec<i8>, Vec<FieldRef>) = output.into_iter().unzip(); - *self = Self::try_new(type_ids, fields)?; + *self = Self(output.into()); Ok(()) } } diff --git a/arrow-schema/src/schema.rs b/arrow-schema/src/schema.rs index b49b3dfd8a..b193f3d18f 100644 --- a/arrow-schema/src/schema.rs +++ b/arrow-schema/src/schema.rs @@ -1478,6 +1478,27 @@ mod tests { ),]), ); + // union members with the same type id must be the same field + let res = Schema::try_merge(vec![ + Schema::new(vec![Field::new_union( + "c1", + vec![0], + vec![Field::new("c11", DataType::Utf8, true)], + UnionMode::Dense, + )]), + Schema::new(vec![Field::new_union( + "c1", + vec![0], + vec![Field::new("c12", DataType::Int64, true)], + UnionMode::Dense, + )]), + ]) + .unwrap_err(); + assert_eq!( + res.to_string(), + "Schema error: Fail to merge schema field 'c12' because type id 0 is already used by field 'c11'" + ); + // incompatible field should throw error assert!( Schema::try_merge(vec