QuakeWang commented on code in PR #54:
URL: https://github.com/apache/paimon-mosaic/pull/54#discussion_r3371460091
##########
core/src/bucket_reader.rs:
##########
@@ -393,16 +480,167 @@ fn reassemble_list_columns(
// where the first child's element_field is NOT itself a complex type
// (complex element_field means it's a nested ARRAY/MAP intermediate).
+ // Skip children that belong to a STRUCT group (will be consumed by
__struct_null__ processing)
+ let is_struct_field = children.iter().enumerate().any(|(ci, c)| {
+ c.parent_logical_col == parent
+ && c.physical_index < phys_idx
+ && c.element_field.name() == "__struct_null__"
+ && !processed[ci]
+ });
+ if is_struct_field {
+ continue;
+ }
+
+ // Check if this is a __struct_null__ child — if so, reassemble the
STRUCT
+ if child.element_field.name() == "__struct_null__" {
+ // Recursively consume STRUCT children (handles nested STRUCTs)
+ let null_bitmap = arrays[phys_idx].clone();
+ let mut cursor = idx + 1;
+ let struct_type = child.element_field.data_type();
+ let (struct_arr, _consumed) = reassemble_struct_element(
+ children,
+ arrays,
+ &mut processed,
+ parent,
+ &mut cursor,
+ struct_type,
+ );
+
+ // Build null buffer from BooleanArray
+ let null_buf = null_bitmap
+ .as_any()
+ .downcast_ref::<BooleanArray>()
+ .and_then(|ba| {
+ let num = ba.len();
+ let mut bm = vec![0u8; num.div_ceil(8)];
+ for i in 0..num {
+ if ba.value(i) {
+ bm[i / 8] |= 1 << (i % 8);
+ }
+ }
+ if bm.iter().all(|&b| b == 0xFF) {
+ None
+ } else {
+ Some(NullBuffer::new(BooleanBuffer::new(
+ Buffer::from_vec(bm),
+ 0,
+ num,
+ )))
+ }
+ });
+
+ let struct_arr = StructArray::new(
+ struct_arr.fields().clone(),
+ struct_arr.columns().to_vec(),
+ null_buf,
+ );
+
+ // Check if this STRUCT is the value part of a MAP (key child
precedes __struct_null__)
+ let map_key_child_idx = if idx > 0 {
+ let prev = idx - 1;
+ if children[prev].parent_logical_col == parent
+ && !processed[prev]
+ && children[prev].element_field.name() != "__struct_null__"
+ && !matches!(
+ children[prev].element_field.data_type(),
+ DataType::List(_) | DataType::Map(_, _)
+ )
+ {
+ Some(prev)
+ } else {
+ None
+ }
+ } else {
+ None
+ };
+
+ if let Some(key_idx) = map_key_child_idx {
+ // MAP<K, STRUCT<...>>: key child + struct value
+ let key_child = &children[key_idx];
+ let keys = arrays[key_child.physical_index].clone();
+
+ let prev_child_is_lengths = children.iter().any(|c| {
+ c.physical_index ==
key_child.physical_index.wrapping_sub(1)
+ && c.parent_logical_col == parent
+ && matches!(
+ c.element_field.data_type(),
+ DataType::List(_) | DataType::Map(_, _)
+ )
+ });
+ let lengths_idx = if prev_child_is_lengths {
+ key_child.physical_index - 1
+ } else {
+ parent
+ };
+
+ let lengths = arrays[lengths_idx].clone();
+ let lengths_rows = lengths.len();
+
+ let key_field = key_child.element_field.as_ref().clone();
+ let val_field = Field::new(
+ "values",
+ DataType::Struct(struct_arr.fields().clone()),
+ true,
+ );
+ let entries_field = Arc::new(Field::new(
+ "entries",
+
DataType::Struct(arrow_schema::Fields::from(vec![key_field, val_field])),
+ false,
+ ));
+ arrays[lengths_idx] = reassemble_map_array(
+ lengths,
+ keys,
+ Arc::new(struct_arr),
+ &entries_field,
+ false,
+ lengths_rows,
+ );
+ processed[key_idx] = true;
+ } else {
+ // ARRAY<STRUCT<...>>: struct values inside a list
+ let prev_child_is_lengths = children.iter().any(|c| {
+ c.physical_index == phys_idx - 1
+ && c.parent_logical_col == parent
+ && matches!(
+ c.element_field.data_type(),
+ DataType::List(_) | DataType::Map(_, _)
+ )
+ });
+ let lengths_idx = if prev_child_is_lengths {
+ phys_idx - 1
+ } else {
+ parent
+ };
+
+ let lengths = arrays[lengths_idx].clone();
+ let lengths_rows = lengths.len();
+ let element_field = Arc::new(Field::new(
Review Comment:
The same issue applies to `ARRAY<STRUCT<...>>`. This hard-codes the list
element field name to `"item"`, but Arrow’s list element field name is part of
the schema, and this project also serializes/preserves it.
For example, with:
```
DataType::List(Arc::new(Field::new("element", struct_type, true)))
```
the read path reconstructs the array with the default `"item"` field name,
which no longer matches the reader schema’s `"element"` field. That causes
`RecordBatch::try_new` to fail with a type mismatch. This should copy the
original list element field name/nullability and only replace the
`DataType::Struct(...)` part.
##########
core/src/bucket_reader.rs:
##########
@@ -393,16 +480,167 @@ fn reassemble_list_columns(
// where the first child's element_field is NOT itself a complex type
// (complex element_field means it's a nested ARRAY/MAP intermediate).
+ // Skip children that belong to a STRUCT group (will be consumed by
__struct_null__ processing)
+ let is_struct_field = children.iter().enumerate().any(|(ci, c)| {
+ c.parent_logical_col == parent
+ && c.physical_index < phys_idx
+ && c.element_field.name() == "__struct_null__"
+ && !processed[ci]
+ });
+ if is_struct_field {
+ continue;
+ }
+
+ // Check if this is a __struct_null__ child — if so, reassemble the
STRUCT
+ if child.element_field.name() == "__struct_null__" {
+ // Recursively consume STRUCT children (handles nested STRUCTs)
+ let null_bitmap = arrays[phys_idx].clone();
+ let mut cursor = idx + 1;
+ let struct_type = child.element_field.data_type();
+ let (struct_arr, _consumed) = reassemble_struct_element(
+ children,
+ arrays,
+ &mut processed,
+ parent,
+ &mut cursor,
+ struct_type,
+ );
+
+ // Build null buffer from BooleanArray
+ let null_buf = null_bitmap
+ .as_any()
+ .downcast_ref::<BooleanArray>()
+ .and_then(|ba| {
+ let num = ba.len();
+ let mut bm = vec![0u8; num.div_ceil(8)];
+ for i in 0..num {
+ if ba.value(i) {
+ bm[i / 8] |= 1 << (i % 8);
+ }
+ }
+ if bm.iter().all(|&b| b == 0xFF) {
+ None
+ } else {
+ Some(NullBuffer::new(BooleanBuffer::new(
+ Buffer::from_vec(bm),
+ 0,
+ num,
+ )))
+ }
+ });
+
+ let struct_arr = StructArray::new(
+ struct_arr.fields().clone(),
+ struct_arr.columns().to_vec(),
+ null_buf,
+ );
+
+ // Check if this STRUCT is the value part of a MAP (key child
precedes __struct_null__)
+ let map_key_child_idx = if idx > 0 {
+ let prev = idx - 1;
+ if children[prev].parent_logical_col == parent
+ && !processed[prev]
+ && children[prev].element_field.name() != "__struct_null__"
+ && !matches!(
+ children[prev].element_field.data_type(),
+ DataType::List(_) | DataType::Map(_, _)
+ )
+ {
+ Some(prev)
+ } else {
+ None
+ }
+ } else {
+ None
+ };
+
+ if let Some(key_idx) = map_key_child_idx {
+ // MAP<K, STRUCT<...>>: key child + struct value
+ let key_child = &children[key_idx];
+ let keys = arrays[key_child.physical_index].clone();
+
+ let prev_child_is_lengths = children.iter().any(|c| {
+ c.physical_index ==
key_child.physical_index.wrapping_sub(1)
+ && c.parent_logical_col == parent
+ && matches!(
+ c.element_field.data_type(),
+ DataType::List(_) | DataType::Map(_, _)
+ )
+ });
+ let lengths_idx = if prev_child_is_lengths {
+ key_child.physical_index - 1
+ } else {
+ parent
+ };
+
+ let lengths = arrays[lengths_idx].clone();
+ let lengths_rows = lengths.len();
+
+ let key_field = key_child.element_field.as_ref().clone();
+ let val_field = Field::new(
Review Comment:
This rebuilds the `MAP<K, STRUCT<...>>` entries/value fields with hard-coded
`"entries"` and `"values"` names. That loses the original Arrow field names
from the schema.
The project already has `test_map_custom_field_names_roundtrip` for
preserving custom map field names in the normal MAP path. When the value is a
`STRUCT`, this new branch changes a schema like:
```
Map("my_entries": Struct("k_custom", "v_custom": Struct(...)))
```
into:
```
Map("entries": Struct("k_custom", "values": Struct(...)))
```
In a local repro, this makes `RecordBatch::try_new` fail with a schema type
mismatch. I think this branch should take the original entries_field and value
field name from the map container type, and only replace/update the value
`Struct` data type and array instead of hard-coding new field names.
--
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]
