feat: Support Map type in Substrait conversions #11129
Conversation
This only supports Map _type_, not Map literals, since Map isn't yet implemented in ScalarValue (apache#11128)
There was a problem hiding this comment.
Thanks (again) @Blizzara
It seems like @goldmedal is working on Map ScalarValue support in #11128 --
#11128 (comment)
So while you are right that this likely isn't super useful on its own, I think it is a step forward towards more full featured map spport
| @@ -2316,6 +2337,19 @@ mod test { | |||
| Field::new_list_field(DataType::Int32, true).into(), | |||
| ))?; | |||
|
|
|||
| round_trip_type(DataType::Map( | |||
| Field::new_struct( | |||
There was a problem hiding this comment.
Maybe we can also add a test using Field::new_map: https://docs.rs/arrow/latest/arrow/datatypes/struct.Field.html#method.new_map
we could do it in a follow on PR too
There was a problem hiding this comment.
Field::new_map is a bit annoying as it creates a map field directly, rather than the inner field of a map.. would be nice to have something like new_list_field but for map, but Arrow doesn't have it.
Anyways I could replace this block or add another one with
round_trip_type(
Field::new_map(
"map",
"entries",
Field::new("key", DataType::Utf8, false).into(),
Field::new("value", DataType::Int32, true).into(),
false,
false,
)
.data_type()
.to_owned(),
)?;
frankly I don't know if it adds much value, but I'm fine either way - do you have a preference?
There was a problem hiding this comment.
If you don't think it is valuable let's not use it
|
Thanks again @Blizzara |
* feat: Support Map type in Substrait conversions This only supports Map _type_, not Map literals, since Map isn't yet implemented in ScalarValue (apache#11128) * fix clippy
Which issue does this PR close?
Closes #.
Rationale for this change
We don't currently support Map types in the Substrait consumer/producer at all. As Map type isn't yet implemented for ScalarValue (#11128), we cannot support Map literals, but we can still enable Map types.
I'm not sure how useful the Map type is w/o the literal support, but I already wrote this code so might as well add it 😅
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?