Jefffrey commented on code in PR #17666:
URL: https://github.com/apache/datafusion/pull/17666#discussion_r2362487157
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -4700,6 +4705,16 @@ select array_to_string(arrow_cast(make_array('h', NULL,
NULL, NULL, 'o'), 'Large
----
h,-,-,-,o nil-2-nil-4-5 1|0|3
+query TTT
+select array_to_string(arrow_cast(make_array('h', NULL, NULL, NULL, 'o'),
'FixedSizeList(5, Utf8)'), ',', '-'),
array_to_string(arrow_cast(make_array(NULL, 2, NULL, 4, 5), 'FixedSizeList(5,
Int64)'), '-', 'nil'), array_to_string(arrow_cast(make_array(1.0, NULL, 3.0),
'FixedSizeList(3, Float64)'), '|', '0');
+----
+h,-,-,-,o nil-2-nil-4-5 1|0|3
+
+query T
+select array_to_string(arrow_cast([arrow_cast([NULL, 'a'], 'FixedSizeList(2,
Utf8)'), NULL], 'FixedSizeList(2, FixedSizeList(2, Utf8))'), ',', '-');
+----
+-,a,-,-
Review 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)`) 🤔
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -370,6 +380,20 @@ pub(super) fn array_to_string_inner(args: &[ArrayRef]) ->
Result<ArrayRef> {
Ok(arg)
}
+ 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)
+ }
Review Comment:
This is handling for when an element of the input list is a `FixedSizeList`
itself
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -184,13 +205,8 @@ impl ScalarUDFImpl for ArrayToString {
&self.signature
}
- 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)
Review Comment:
We don't need to guard here since type signature should enforce for us now
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -284,16 +300,10 @@ impl ScalarUDFImpl for StringToArray {
}
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- 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,
+ ))))
Review Comment:
This is for string to array, but is similar to above in that type signature
already ensures the arguments are string
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -471,29 +494,8 @@ pub(super) fn array_to_string_inner(args: &[ArrayRef]) ->
Result<ArrayRef> {
with_null_string,
)?
}
- _ => {
- 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)
- }
Review 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 🤔
--
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]
