Binary string (`BYTEA`, `Binary`) concatenation

[kawadakk]

Is your feature request related to a problem or challenge?

Currently there doesn't seem to be a direct way to concatenate binary strings in SQL expressions.

• lhs || rhs does not accept Binary arguments.
• concat(lhs, rhs) coerces both sides to Utf8, which does not work if they are not valid UTF-8 strings.
• array_concat(lhs, rhs) only works for List, not Binary.
• decode(encode(lhs, 'hex') || encode(rhs, 'hex'), 'hex') works, but this is verbose and inefficient.

Describe the solution you'd like

The || operator could be extended to support Binary arguments, following the behavior of other SQL engines, such as SQLite and PostgreSQL.

Describe alternatives you've considered

No response

Additional context

No response

[alamb]

I agree this still seems not to be supported

> select arrow_typeof(x'1234');
+-------------------------------+
| arrow_typeof(Binary("18,52")) |
+-------------------------------+
| Binary                        |
+-------------------------------+
1 row(s) fetched.
Elapsed 0.003 seconds.

> select x'1234' || x'5678';
Error during planning: Cannot infer common string type for string concat operation Binary || Binary

[jayzhan211]

ArrayConcat use signature variadic_any which should be changed to something in ArraySignature

[Jefffrey]

Some pointers:

Look here for the existing StringConcat operator:

datafusion/datafusion/expr-common/src/operator.rs

Lines 83 to 84 in a02e683

	/// String concat
	StringConcat,

We should look at places it is used and either try to extend it to include binary (although it might be weird since the name is StringConcat unless we rename it) or potentially introduce a new operator BinaryConcat. We should also consider the UDF:

https://github.com/apache/datafusion/blob/a02e6836e7d449008eef592b177ea282595e027a/datafusion/functions/src/string/concat.rs

One thing to keep in mind is how we handle binary || string concat (i.e. which common type we coerce to); we can't assume all binary is a valid string, but strings are always valid binary.

[devanshu0987]

One thing to keep in mind is how we handle binary || string concat (i.e. which common type we coerce to); we can't assume all binary is a valid string, but strings are always valid binary.

devanshu=# SELECT 'abc'::text || '\x00'::bytea AS result,  pg_typeof('abc'::text || '\x00'::bytea) AS concat_result;
 result  | concat_result 
---------+---------------
 abc\x00 | text
(1 row)

I validated the rules in Postgres 18. It coerces it to Text type regardless of whether the binary string is valid or not.
bytea is coerced to text via its textual representation.

Can we follow the same for Data Fusion?

[Jefffrey]

I don't think we can follow that behaviour for DataFusion, because our version of casting binary to string will try to interpret the actual bytes as UTF8, whereas it seems postgres will convert it to the hex representation?

[devanshu0987]

it seems postgres will convert it to the hex representation

Yes. I used Cursor to find the implementation. Seems it does a hex_encode.
src/backend/utils/adt/bytea.c

Datum
byteaout(PG_FUNCTION_ARGS)
{
	bytea	   *vlena = PG_GETARG_BYTEA_PP(0);
	char	   *result;
	char	   *rp;

	if (bytea_output == BYTEA_OUTPUT_HEX)
	{
		/* Print hex format */
		rp = result = palloc(VARSIZE_ANY_EXHDR(vlena) * 2 + 2 + 1);
		*rp++ = '\\';
		*rp++ = 'x';
		rp += hex_encode(VARDATA_ANY(vlena), VARSIZE_ANY_EXHDR(vlena), rp);
	}

[Jefffrey]

I think we can proceed with assumption of preferring binary over string, and maybe with a PR up there can be discussion from others on how it looks and if it's preferred to instead cast to string in cases of binary || string 🤔

[theirix]

It's safer to make a proper binary concatenation for a binary/string mix. Spark does it more safely this way, while the majority (Postgres, DuckDB, and Snowflake) actually prefer casting to text for historical reasons.

I can draft a PR with an experimental implementation for both binary/string and binary/binary to verify that it fits our expectations.

[alamb]

In my opinion we should follow what the other systems do rather than inventing our own semantics

[theirix]

What do you think of having a dialect-dependent implementation via customising ExprPlanner? For Spark/Databricks we plan a binary | binary -> text, while for others, we use binary. So we can have expected behaviour for every platform.

[Jefffrey]

What do you think of having a dialect-dependent implementation via customising ExprPlanner? For Spark/Databricks we plan a binary | binary -> text, while for others, we use binary. So we can have expected behaviour for every platform.

Do we have existing precedent for such customization at that level?

[theirix]

What do you think of having a dialect-dependent implementation via customising ExprPlanner? For Spark/Databricks we plan a binary | binary -> text, while for others, we use binary. So we can have expected behaviour for every platform.

Do we have existing precedent for such customization at that level?

Not quiet. The idea is to make a plan using two UDFs in ExprPlanner::plan_binary_op - one ConcatBinary with text-to-binary and the classical Concat. The planner chooses between them based on a dialect. A good example is NestedFunctionPlanner::plan_binary_op

Since it had not been done before, I stumbled upon maintaining an actual config with a dialect in the expression planner, so we can react to set dialect dynamically. A session state cannot be passed to SessionStateBuilder::with_default_features, where default planners are instantiated early, while the state is rebuilt often via new_from_existing. I believe it could be done with some redesign, but it's worth confirming if it's worth it.

[Jefffrey]

Maybe it's best to keep this simple and follow what Postgres does for now; we do have precedent for having Spark specific functions so if we want to follow Spark we could have that behaviour in the datafusion-spark crate 🤔