Skip to content

refactor: improve display formatting for Union #8529

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 4, 2025
Merged

refactor: improve display formatting for Union #8529

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9b64973
refactor: improve display formatting for Struct
Weijun-H Oct 1, 2025
586236d
test: add display test for Union data type
Weijun-H Oct 1, 2025
ca49a25
chore: Add a test for metadata
Weijun-H Oct 2, 2025
3d5af15
chore: Add a test for struct display and use semicomma for field display
Weijun-H Oct 2, 2025
8b4cfe6
chore: clippy
Weijun-H Oct 2, 2025
5a92730
fix: Still keep comma to seperate field
Weijun-H Oct 2, 2025
92868e4
Merge branch 'main' into 8351-better-struct-display
alamb Oct 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion arrow-schema/src/datatype_display.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,24 @@ impl fmt::Display for DataType {
Ok(())
}
Self::Union(union_fields, union_mode) => {
write!(f, "Union({union_fields:?}, {union_mode:?})")
write!(f, "Union({union_mode:?}, ")?;
if !union_fields.is_empty() {
let fields_str = union_fields
.iter()
.map(|v| {
let type_id = v.0;
let field = v.1;
let maybe_nullable = if field.is_nullable() { "nullable " } else { "" };
let data_type = field.data_type();
let metadata_str = format_metadata(field.metadata());
format!("{type_id:?}: {maybe_nullable}{data_type}{metadata_str}")
})
.collect::<Vec<_>>()
.join(", ");
Copy link
Member

@martin-g martin-g Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to use ; as a separator ?
This way the , used to separate the metadata won't confuse the readers.

Copy link
Member Author

@Weijun-H Weijun-H Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch

Copy link
Member Author

@Weijun-H Weijun-H Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some investigation, I think we should continue using commas to separate different fields, following the DuckDB SQL style. For metadata, since it is displayed with brackets, it should not be too confusing.

write!(f, "{fields_str}")?;
}
write!(f, ")")?;
Ok(())
}
Self::Dictionary(data_type, data_type1) => {
write!(f, "Dictionary({data_type}, {data_type1})")
Expand Down Expand Up @@ -248,4 +265,22 @@ mod tests {
"FixedSizeList(4 x nullable Int32, metadata: {\"key2\": \"value2\"})";
assert_eq!(fixed_size_metadata_string, expected_metadata_string);
}

#[test]
fn test_display_union() {
let fields = vec![
Field::new("a", DataType::Int32, false),
Field::new("b", DataType::Utf8, true),
];
let type_ids = vec![0, 1];
let union_fields = type_ids
.into_iter()
.zip(fields.into_iter().map(Arc::new))
.collect();

let union_data_type = DataType::Union(union_fields, crate::UnionMode::Sparse);
let union_data_type_string = union_data_type.to_string();
let expected_string = "Union(Sparse, 0: Int32, 1: nullable Utf8)";
assert_eq!(union_data_type_string, expected_string);
}
}
Loading