alamb commented on code in PR #14378:
URL: https://github.com/apache/datafusion/pull/14378#discussion_r1936300052
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -2848,6 +2848,47 @@ select array_concat([]);
----
[]
+# Concatenating strings arrays
+query ?
+select array_concat(
+ ['1', '2'],
+ ['3']
+);
+----
+[1, 2, 3]
+
+# Concatenating string arrays
+query ?
+select array_concat(
+ [arrow_cast('1', 'LargeUtf8'), arrow_cast('2', 'LargeUtf8')],
+ [arrow_cast('3', 'LargeUtf8')]
+);
+----
+[1, 2, 3]
+
+# Concatenating stringview
+query ?
+select array_concat(
+ [arrow_cast('1', 'Utf8View'), arrow_cast('2', 'Utf8View')],
+ [arrow_cast('3', 'Utf8View')]
+);
+----
+[1, 2, 3]
+
+# Concatenating Mixed types (doesn't work)
Review Comment:
Both of these queries fail on main though the errors are different for the
string view one (see the first commit of this PR)
It seems to me like this display is 🤮 due to the `Display` impl of
`DataType` being crap.
I'll file an upstream ticket to make this easier to understand
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -2848,6 +2848,47 @@ select array_concat([]);
----
[]
+# Concatenating strings arrays
+query ?
+select array_concat(
+ ['1', '2'],
+ ['3']
+);
+----
+[1, 2, 3]
+
+# Concatenating string arrays
+query ?
+select array_concat(
+ [arrow_cast('1', 'LargeUtf8'), arrow_cast('2', 'LargeUtf8')],
+ [arrow_cast('3', 'LargeUtf8')]
+);
+----
+[1, 2, 3]
+
+# Concatenating stringview
Review Comment:
these three queries work fine on main
##########
datafusion/functions-nested/src/concat.rs:
##########
@@ -276,25 +276,32 @@ impl ScalarUDFImpl for ArrayConcat {
let mut expr_type = DataType::Null;
let mut max_dims = 0;
for arg_type in arg_types {
- match arg_type {
- DataType::List(field) => {
- if !field.data_type().equals_datatype(&DataType::Null) {
- let dims = list_ndims(arg_type);
- expr_type = match max_dims.cmp(&dims) {
- Ordering::Greater => expr_type,
- Ordering::Equal => get_wider_type(&expr_type,
arg_type)?,
- Ordering::Less => {
- max_dims = dims;
- arg_type.clone()
- }
- };
+ let DataType::List(field) = arg_type else {
+ return plan_err!(
+ "The array_concat function can only accept list as the
args."
+ );
+ };
+ if !field.data_type().equals_datatype(&DataType::Null) {
+ let dims = list_ndims(arg_type);
+ expr_type = match max_dims.cmp(&dims) {
+ Ordering::Greater => expr_type,
+ Ordering::Equal => {
+ if expr_type == DataType::Null {
+ arg_type.clone()
+ } else if !expr_type.equals_datatype(arg_type) {
+ return plan_err!(
+ "It is not possible to concatenate arrays of
different types. Expected: {}, got: {}", expr_type, arg_type
Review Comment:
the implementation of `array_concat` requires the field types of `ListArray`
to be the same and errors at runtime if they aren't
Previously the return type was calculated only to get a runtime error. I
simply moved the check to planning time
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
