datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

Use non-comparison coercion for `Coalesce` or even avoid implicit casting for `Coalesce`

Open jayzhan211 opened this issue 1 year ago • 4 comments

Is your feature request related to a problem or challenge?

What does Coalesce do

The coalesce function implicitly coerces types with Signature::VariadicEqual which has comparison_coercion internally, and the coercion is taken considered for all the columns, not only those we need, which gives us back unexpected casting results.

Coerce types after first non-null values are known

We can see the following example,

statement ok
create table test1 (a int, b int) as values (null, 3), (2, null);

# test coercion string
query TT
select 
	coalesce(a, b, 'none_set'),
	arrow_typeof(coalesce(a, b, 'none_set'))
from test1;
----
3 Utf8
2 Utf8

Since they are coerced to Utf8, so we get 3 and 2 with Utf8.

Ideally, If we take the first non-null value, we should expect to get Int, not Utf8.

another example, dict is cast to Int64

query IT
select 
	coalesce(arrow_cast(123, 'Dictionary(Int32, Int8)'), 34),
	arrow_typeof(coalesce(arrow_cast(123, 'Dictionary(Int32, Int8)'), 34))
----
123 Int64

The reason we need coercion is that it is possible that we have different types for different columns. Coerce them can help we get the final single type.

statement ok
create table test1 as values 
	(arrow_cast(1, 'Int8'), arrow_cast(2, 'Int32')),
	(null, arrow_cast(3, 'Int32'))

query IT
select 
	coalesce(column1, column2),
	arrow_typeof(coalesce(column1, column2))
from test1;
----
1 Int32
3 Int32

We get (1, Int8) and (3, Int32) for respective row, and finally cast them to I32.

I suggest that we apply coercion after we collect those first non-null values for each row.

Use non-comparison coercion

Comparision coercion (fn comparison_coercion) vs non-comparison coercion (fn coerced_from)

Those two logic are quite different, comparison coercion is for comparision. For example, compare dict with dict returns value type, since dict key is not important. Compare i64 with u64 we fallback to i64 because we don't have i128, even there is possible of lossy if you have large U64 value, but most of the cases like U64(1) and I64(1) is comparable, we will not block for those edge cases in comparison. And, there might be more.

Given the difference between these two coercion, I think non-comparison coercion is more suitable for Coalesce function. ~Btw, I think make_array should switch to non-comparison coercion too~. make_array should do comparison coercion, following what Duckdb do

I suggest we switch VariadicEqual to non-comparison coercion or introduce another signature VariadicEqualNonCompare if comparison coercion is needed somewhere.

I think Int64 is a big surprise that we should avoid.

query IT
select 
	coalesce(arrow_cast(3, 'Dictionary(Int32, Int32)'), arrow_cast(3, 'Dictionary(Int32, Int64)')),
	arrow_typeof(coalesce(arrow_cast(3, 'Dictionary(Int32, Int32)'), arrow_cast(3, 'Dictionary(Int32, Int64)')));
----
3 Int64

Maybe disable implicit coercion for Coalesce function?

I'm not sure why is coalesce function introduce implicit coercion, but I found that Postgres and Duckdb does not do implicit casting for Coalesce, maybe we should follow them?

DuckDB errors

D create table t1 (a integer, b varchar);
D insert into t1 values (1, 'a');
D insert into t1 values (null, 'b');
D select coalesce(a, b) from t1;
Error: Binder Error: Cannot mix values of type INTEGER and VARCHAR in COALESCE operator - an explicit cast is required

Postgres Error

postgres=# create table t2(a int, b varchar);
CREATE TABLE
postgres=# insert into t2 values (1, 'a');
INSERT 0 1
postgres=# insert into t2 values (null, 'b');
INSERT 0 1
postgres=# select coalesce(a, b) from t2;
ERROR:  COALESCE types integer and character varying cannot be matched
LINE 1: select coalesce(a, b) from t2;

Describe the solution you'd like

  1. Avoid casting for coalesce at all
  2. Consider coercion after collecting known results if we need casting.
  3. Switch VariadicEqual to non-comparison coercion.

Describe alternatives you've considered

No response

Additional context

No response

Related issue that has coercion issue from Coalesce #10221 Part of the idea #10241

jayzhan211 avatar Apr 27 '24 06:04 jayzhan211

If possible, I think we should follow the model of existing implementations here (rather than invent DataFusion specific semants)

Specifically if we are going to change the semantics of coalsce I think we should follow either postgres or spark's behavior -- I haven't done the research to know how close/far what DataFusion does compared to those systems

alamb avatar Apr 27 '24 11:04 alamb

~Alright, so I think we should remove casting logic for coalesce, which follows what Postgres and DuckDB do. Returns error if there are mixed types for different columns~.

It seems Postgres has casting for some of the types too.

jayzhan211 avatar Apr 27 '24 12:04 jayzhan211

Plan:

  1. I think we should change coerced_from to can_cast_types here, pre-computed reasonable valid types with TypeSignature in get_valid_types. https://github.com/apache/datafusion/blob/ed146828fdeb9784eb2963ae255909b9ac53c61b/datafusion/expr/src/type_coercion/functions.rs#L67-L71

https://github.com/apache/datafusion/blob/ed146828fdeb9784eb2963ae255909b9ac53c61b/datafusion/expr/src/type_coercion/functions.rs#L82-L261

  1. Non-comparison coercion: type resolution coercion for Coalesce, MakeArray, and so on.

https://www.postgresql.org/docs/current/typeconv-union-case.html

  1. Deprecate/Remove coerced_from at the end!

jayzhan211 avatar Apr 28 '24 12:04 jayzhan211

BTW this is a really nicely written ticket

alamb avatar May 01 '24 19:05 alamb