unexpected output for `concat` for arrays

[comphead]

Describe the bug

concat builtin in function should concat arrays like array_concat, currently the output is wrong

> select concat(make_array(1, 2, 3), make_array(4, 5));
+------------------------------------------------------------------------------+
| concat(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(4),Int64(5))) |
+------------------------------------------------------------------------------+
| [1, 2, 3][4, 5]                                                              |
+------------------------------------------------------------------------------+
1 row(s) fetched. 
Elapsed 0.012 seconds.

> select array_concat(make_array(1, 2, 3), make_array(4, 5));
+------------------------------------------------------------------------------------+
| array_concat(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(4),Int64(5))) |
+------------------------------------------------------------------------------------+
| [1, 2, 3, 4, 5]                                                                    |
+------------------------------------------------------------------------------------+
1 row(s) fetched. 
Elapsed 0.014 seconds.

concat for array should return same as array_concat

To Reproduce

No response

Expected behavior

No response

Additional context

No response

[petern48]

take

[petern48]

I see that the string concat function is being called instead of function-nested array_concat one. Adding a concat alias to array_concat technically solves the problem. Is that what we want? Seems reasonable to me, but I just didn't see any signs that we want / expected this to be already be an alias.

[comphead]

Thanks @petern48 for looking into this issue. I'm not sure how would be easy to make an alias depending on the signature. in the end of the day we need to have concat with arrays working properly and this is current behavior in DuckDB.

Naive solution is to replace concat function to array_concat in the planner depending on the input

[petern48]

I see, so it sounds like we want to use the current concat implementation only for strings and then use array_concat implementation for the other data types?

I tried finding an approach where we didn't have to replace the function call in the planner. I tried adding concat as an alias to array_concat, hoping that concat.rs would be called for strings and array_concat would work for everything else, but array_concat seemed to just override the concat.rs udf entirely (saying there's no valid kernel for Utf8 args), so the existing concat tests failed.

Anyways, given my current bandwidth, I'd rather leave this for someone else to debug / propose a solution. Could you unassign me @comphead? Doesn't seem like I can do it myself.

[divyanshmrawal]

We have to use array_concat for concatenating arrays because concat is used for strings, and array_concat is used for concatenating arrays.

[EeshanBembi]

I'd like to take a shot at this issue. After examining the codebase, I think there's a clean solution that doesn't require planner changes or function aliases.

The key insight is that we can modify the existing concat function to detect when array types are passed and delegate to the array_concat logic internally, while preserving all existing string concatenation behavior.

Proposed approach:

1. Extend the concat function signature to accept array types (List, LargeList,
   FixedSizeList) in addition to string types
2. Add type detection logic in invoke_with_args - if any argument is an array
   type, delegate to array_concat_inner
3. Keep all existing string logic unchanged for backward compatibility

The implementation would be contained within
/datafusion/functions/src/string/concat.rs with minimal changes to the existing
logic flow.

@comphead Would this approach work?

[comphead]

Thanks @EeshanBembi it might actually work, would be nice if you create a draft PR and explore how that would work.

[comphead]

I played with replacement in the planner, but somehow I dont feel like this is a good idea.
@EeshanBembi I have attached a PR just for reference, please feel free to take tests from the PR, tests for both DF concat and Spark should pass(they are slightly different)