Copilot commented on code in PR #9524:
URL: https://github.com/apache/arrow-rs/pull/9524#discussion_r2901180084
##########
arrow-schema/src/field.rs:
##########
@@ -1461,4 +1473,58 @@ mod test {
assert_binary_serde_round_trip(field)
}
+
+ #[test]
+ fn test_merge_compound_with_null() {
+ // Struct + Null
+ let mut field = Field::new(
+ "s",
+ DataType::Struct(Fields::from(vec![Field::new("a",
DataType::Int32, false)])),
+ false,
+ );
+ field
+ .try_merge(&Field::new("s", DataType::Null, true))
+ .expect("Struct should merge with Null");
+ assert!(field.is_nullable());
+ assert!(matches!(field.data_type(), DataType::Struct(_)));
+
+ // List + Null
+ let mut field = Field::new(
+ "l",
+ DataType::List(Field::new("item", DataType::Utf8, false).into()),
+ false,
+ );
+ field
+ .try_merge(&Field::new("l", DataType::Null, true))
+ .expect("List should merge with Null");
+ assert!(field.is_nullable());
+ assert!(matches!(field.data_type(), DataType::List(_)));
+
+ // LargeList + Null
+ let mut field = Field::new(
+ "ll",
+ DataType::LargeList(Field::new("item", DataType::Utf8,
false).into()),
+ false,
+ );
+ field
+ .try_merge(&Field::new("ll", DataType::Null, true))
+ .expect("LargeList should merge with Null");
+ assert!(field.is_nullable());
+ assert!(matches!(field.data_type(), DataType::LargeList(_)));
+
+ // Union + Null
+ let mut field = Field::new(
+ "u",
+ DataType::Union(
+ UnionFields::try_new(vec![0], vec![Field::new("f",
DataType::Int32, false)])
+ .unwrap(),
+ UnionMode::Dense,
+ ),
+ false,
+ );
+ field
+ .try_merge(&Field::new("u", DataType::Null, true))
+ .expect("Union should merge with Null");
Review Comment:
The Union + Null subtest doesn't assert that the field became nullable after
the merge. Please add an `assert!(field.is_nullable())` here (similar to the
Struct/List/LargeList cases) so the test actually validates the new
Union-specific `DataType::Null` handling.
```suggestion
.expect("Union should merge with Null");
assert!(field.is_nullable());
```
##########
arrow-schema/src/field.rs:
##########
@@ -1461,4 +1473,58 @@ mod test {
assert_binary_serde_round_trip(field)
}
+
+ #[test]
+ fn test_merge_compound_with_null() {
+ // Struct + Null
+ let mut field = Field::new(
+ "s",
Review Comment:
PR description says the added test covers merging with Null "in both
directions", but this test only exercises `compound.try_merge(Null)`. Consider
adding a reverse-direction assertion (`Null.try_merge(compound)`) for at least
one (or all) of Struct/List/LargeList/Union to match the stated coverage and
guard the `DataType::Null` self-branch behavior.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
