moraistejerina commented on code in PR #25015:
URL: https://github.com/apache/datafusion/pull/25015#discussion_r3950152256
##########
datafusion/substrait/src/logical_plan/consumer/types.rs:
##########
@@ -349,7 +349,12 @@ fn from_substrait_struct_type(
for (i, f) in s.types.iter().enumerate() {
let name = next_struct_field_name(i, dfs_names, name_idx)?;
let data_type = from_substrait_type(consumer, f, dfs_names, name_idx)?;
- let field = Field::new(name, data_type, type_is_nullable(f)?);
+ let mut field = Field::new(name, data_type, type_is_nullable(f)?);
+ if let Some(r#type::Kind::UserDefined(udt)) = &f.kind
+ && let Some(metadata) =
consumer.consume_user_defined_type_metadata(udt)?
+ {
+ field = field.with_metadata(metadata);
+ }
fields.push(field);
Review Comment:
Went for an approach similar to the last option by adding the private method
`substrait_type_to_field`, in order not to change the behavior of the public
function `field_from_substrait_type`. This new method is responsible only for
adding metadata.
`substrait_type_to_field` wraps `from_substrait_type` and is the only place
in the crate calling `consume_type_metadata`/`with_metadata`. Every site
building a Field from a Type goes through it now: both
`field_from_substrait_type*` entry points, the four sites you listed (list
element, map key, map value, EmptyMap literal), and struct fields.
`from_substrait_type` stays untouched, still public, still returns a bare
DataType to avoid a domino effect on its other callers.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
