Support FixedSizeList for array_to_string #17666
Conversation
github-actions
bot
added
sqllogictest
| fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> { | ||
| Ok(match arg_types[0] { | ||
| List(_) | LargeList(_) | FixedSizeList(_, _) => Utf8, | ||
| _ => { | ||
| return plan_err!("The array_to_string function can only accept List/LargeList/FixedSizeList."); | ||
| } | ||
| }) | ||
| fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> { | ||
| Ok(Utf8) |
There was a problem hiding this comment.
We don't need to guard here since type signature should enforce for us now
| Ok(match arg_types[0] { | ||
| Utf8 | Utf8View | LargeUtf8 => { | ||
| List(Arc::new(Field::new_list_field(arg_types[0].clone(), true))) | ||
| } | ||
| _ => { | ||
| return plan_err!( | ||
| "The string_to_array function can only accept Utf8, Utf8View or LargeUtf8." | ||
| ); | ||
| } | ||
| }) | ||
| Ok(List(Arc::new(Field::new_list_field( | ||
| arg_types[0].clone(), | ||
| true, | ||
| )))) |
There was a problem hiding this comment.
This is for string to array, but is similar to above in that type signature already ensures the arguments are string
| FixedSizeList(..) => { | ||
| let list_array = as_fixed_size_list_array(&arr)?; | ||
| for i in 0..list_array.len() { | ||
| compute_array_to_string( | ||
| arg, | ||
| list_array.value(i), | ||
| delimiter.clone(), | ||
| null_string.clone(), | ||
| with_null_string, | ||
| )?; | ||
| } | ||
|
|
||
| Ok(arg) | ||
| } |
There was a problem hiding this comment.
This is handling for when an element of the input list is a FixedSizeList itself
| _ => { | ||
| let mut arg = String::from(""); | ||
| let mut res: Vec<Option<String>> = Vec::new(); | ||
| // delimiter length is 1 | ||
| assert_eq!(delimiters.len(), 1); | ||
| let delimiter = delimiters[0].unwrap(); | ||
| let s = compute_array_to_string( | ||
| &mut arg, | ||
| Arc::clone(arr), | ||
| delimiter.to_string(), | ||
| null_string, | ||
| with_null_string, | ||
| )? | ||
| .clone(); | ||
|
|
||
| if !s.is_empty() { | ||
| let s = s.strip_suffix(delimiter).unwrap().to_string(); | ||
| res.push(Some(s)); | ||
| } else { | ||
| res.push(Some(s)); | ||
| } | ||
| StringArray::from(res) | ||
| } |
There was a problem hiding this comment.
I removed this arm as the array (which is the first argument) must be a List type (List, Fixed or Large) by this point so this arm didn't make sense; even before my changes I don't think this arm was ever reachable as the return_type() function guarded arg[0] to be a list type 🤔
| query T | ||
| select array_to_string(arrow_cast([arrow_cast([NULL, 'a'], 'FixedSizeList(2, Utf8)'), NULL], 'FixedSizeList(2, FixedSizeList(2, Utf8))'), ',', '-'); | ||
| ---- | ||
| -,a,-,- |
There was a problem hiding this comment.
This is interesting; it's formatting [[NULL, 'a'], NULL] to become this; note three - implying three nulls. I'm not sure if this is desired or if it should instead be -,a,- since the second element of the parent array is null (current code considers it to be two nulls, I guess because it represents a FixedSizeList(2)) 🤔
There was a problem hiding this comment.
I'll look into this as a follow-on issue
Which issue does this PR close?
Part of #8249
Rationale for this change
Was checking through array functions and found
array_to_stringdoesn't handleFixedSizeListarrays. Fix so it does.What changes are included in this PR?
array_to_stringto allow coercion fromFixedSizeListtoListFixedSizeListas one of the child elements of the input listAre these changes tested?
Added SLT cases
Are there any user-facing changes?
No