Coerce Array inner types #13452

blaginin · 2024-11-16T17:11:32Z

Which issue does this PR close?

Rationale for this change

Currently, we don't attempt to coerce inner list types, and so this works (although it shouldn't)

SELECT make_array(2) x UNION ALL SELECT make_array(now()) x;

What changes are included in this PR?

Now we’ll try to coerce inner types instead of just using the type of the first array.

Are these changes tested?

Yes, added a test

Are there any user-facing changes?

No

findepi

looking good!

you can add tests in datafusion/sqllogictest/test_files/union.slt

datafusion/expr-common/src/type_coercion/binary.rs

blaginin · 2024-11-16T17:54:44Z

That was a very fast review 😅🙏

datafusion/sqllogictest/test_files/union.slt

datafusion/expr-common/src/type_coercion/binary.rs

findepi · 2024-11-17T13:28:50Z

datafusion/expr-common/src/type_coercion/binary.rs

@@ -1138,27 +1138,44 @@ fn numeric_string_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<D
    }
 }

+fn coerce_list_children(lhs_field: &FieldRef, rhs_field: &FieldRef) -> Option<FieldRef> {
+    Some(Arc::new(
+        Arc::unwrap_or_clone(Arc::clone(lhs_field)).with_data_type(comparison_coercion(


comparison_coercion

should we use type_union_resolution here?

So both will work, but IMO the current version is a bit better as it makes code aligned with the dictionally behaviour (dictionary_comparison_coercion uses comparison_coercion)

I think we should use type_union_resolution here since we need to preserve dictionary when we union two array. But comparison coercion doesn't preserve dictionary type, only inner type matters

I may have misunderstood your point, but I am pretty sure comparison_coercion preserves dicts:

datafusion/datafusion/expr-common/src/type_coercion/binary.rs

Line 637 in 22c1f54

.or_else(|| dictionary_comparison_coercion(lhs_type, rhs_type, true))

For example, this query correctly casts Dict(Utf8) to Dict(LargeUtf8)

select arrow_typeof(x) from (select make_array(arrow_cast('a', 'Dictionary(Int8, Utf8)')) x UNION ALL SELECT make_array(arrow_cast('b', 'Dictionary(Int8, LargeUtf8)'))) x;

Also, I think type_union_resolution is a bit more limiting than comparison_coercion, and so, for example, if I switch to it, the following two queries will stop working:

-- type_union_resolution can't cast nulls select make_array(arrow_cast('a', 'Utf8')) x UNION ALL SELECT make_array(NULL) x; -- type_union_resolution can't handle large lists (or fixed lists) select make_array(make_array(1)) x UNION ALL SELECT make_array(arrow_cast(make_array(-1), 'LargeList(Int8)')) x;

query T select arrow_typeof(x) from (select make_array(arrow_cast('a', 'Dictionary(Int8, Utf8)')) x UNION ALL SELECT make_array(arrow_cast('b', 'Dictionary(Int8, LargeUtf8)'))) x; ---- List(Field { name: "item", data_type: LargeUtf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }) List(Field { name: "item", data_type: LargeUtf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} })

Shouldn't the result be like

query T select arrow_typeof(x) from (select make_array(arrow_cast('a', 'Dictionary(Int8, Utf8)')) x UNION ALL SELECT make_array(arrow_cast('b', 'Dictionary(Int8, LargeUtf8)'))) x; ---- List(Field { name: "item", data_type: Dict(), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }) List(Field { name: "item", data_type: Dict(), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} })

Also, I think type_union_resolution is a bit more limiting than comparison_coercion, and so, for example, if I switch to it, the following two queries will stop working

It indicates we need to handle more coercion for type_union_resolution.

dictionary_comparison_coercion should not preserve dict if both are dict. There is a logic to optionally preserve dict if one of them is dict, one is not

greatest(x, y, z) should be equivalent to select max(a) from select x as a union all select y as a select z as a and also should be equivalent to select array_max(array[x, y, z])

all these should converge x, y, z to the common super type, and i believe type_union_resolution is supposedly doing just that.

Okay, that's fair! I've switched to type_union_resolution and will create a ticket to handle large arrays and nulls

datafusion/expr-common/src/type_coercion/binary.rs

findepi · 2024-11-17T13:32:49Z

datafusion/expr-common/src/type_coercion/binary.rs

@@ -1138,27 +1138,44 @@ fn numeric_string_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<D
    }
 }

+fn coerce_list_children(lhs_field: &FieldRef, rhs_field: &FieldRef) -> Option<FieldRef> {
+    Some(Arc::new(
+        Arc::unwrap_or_clone(Arc::clone(lhs_field)).with_data_type(comparison_coercion(


@alamb can there be a problem if we coerce two lists and they have different field names?

@blaginin the resulting field nullability should be OR of nullability of sources

#13468 seems related to this

Given that this PR doesn’t change names behaviour, let’s go as is and then fix it separately in that PR you highlighted?

i am fine assuming the answer to this question is 'nope':

can there be a problem if we coerce two lists and they have different field names?

i hope it is 'nope, no problem'
@alamb @jayzhan211 please confirm

I don't think there is any problem in theory. I am a little fuzzy about what the semantic meaning of the Field's name in a DataType::List means -- sometimes the code is overly pedantic when comparing but I think semantically two lists are the same if their element's types are the same (and nullness and metadata). I don't think the field name should be compared

However, I remember there have been issues before on this point

I think #13481 is a complain that list item name isn't preserved in some cases. I don't know when it would matter though. From SQL perspective, it shouldn't.

datafusion/expr-common/src/type_coercion/binary.rs

jayzhan211

👍

type_union_resolution

alamb

Thank you @findepi and @blaginin and @jayzhan211 -- I think this looks reasonable to me

Let's wait to see if @jayzhan211 wants to review again as well

alamb · 2024-11-19T12:17:14Z

datafusion/expr-common/src/type_coercion/binary.rs

@@ -1138,27 +1138,47 @@ fn numeric_string_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<D
    }
 }

+fn coerce_list_children(lhs_field: &FieldRef, rhs_field: &FieldRef) -> Option<FieldRef> {
+    Some(Arc::new(


Could you add a comment here explaining why this function is needed -- specifically that it is setting the DataType / field name correctly?

…coerce-arrays-inner-types

blaginin · 2024-11-19T19:44:38Z

datafusion/functions-nested/src/make_array.rs

@@ -198,7 +198,6 @@ pub(crate) fn make_array_inner(arrays: &[ArrayRef]) -> Result<ArrayRef> {
            let array = new_null_array(&DataType::Int64, length);
            Ok(Arc::new(array_into_list_array_nullable(array)))
        }
-        LargeList(..) => array_array::<i64>(arrays, data_type),


I don't think this line was correct. According to the signature, the function output is always a List. However, this one sometimes returns a LargeList.

I added a test to illustrate this:

datafusion/datafusion/sqllogictest/test_files/array.slt

Line 1160 in 91b468d

select make_array(make_array(1), arrow_cast(make_array(-1), 'LargeList(Int8)'))

query ? select make_array(make_array(1), arrow_cast(make_array(-1), 'LargeList(Int8)')); ---- [[1], [-1]] query T select arrow_typeof(make_array(make_array(1), arrow_cast(make_array(-1), 'LargeList(Int8)'))); ---- List(Field { name: "item", data_type: LargeList(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} })

It seems correct 🤔 ?

I see. You fix it :) Thanks

datafusion/sqllogictest/test_files/array.slt

findepi · 2024-11-20T09:49:24Z

datafusion/expr-common/src/type_coercion/binary.rs

-        (FixedSizeList(_, _), List(_)) => Some(rhs_type.clone()),
+        (
+            LargeList(lhs_field),
+            List(rhs_field) | LargeList(rhs_field) | FixedSizeList(rhs_field, _),


Let's maybe add listview to the mix. it's hard to test, but why not be future-proof here?

let's maybe reorder clauses so that it's clear that any list types are handled

match (lhs_type, rhs_type) { // Coerce to the left side FixedSizeList type if the list lengths are the same, // otherwise coerce to list with the left type for dynamic length (FixedSizeList(lhs_field, ls), FixedSizeList(rhs_field, rs)) if ls == rs => Some( FixedSizeList(coerce_list_children(lhs_field, rhs_field)?, *rs), ), // Left is a LargeList[View] or right is a LargeList[View] ( LargeList(lhs_field) | LargeListView(lhs_field), List(rhs_field) | ListView(rhs_field) | LargeList(rhs_field) | LargeListView(rhs_field) | FixedSizeList(rhs_field, _), ) | ( List(lhs_field) | ListView(lhs_field) | FixedSizeList(lhs_field, _) | LargeList(lhs_field) | LargeListView(lhs_field), LargeList(rhs_field) | LargeListView(rhs_field), ) => Some(LargeList(coerce_list_children(lhs_field, rhs_field)?)), // Left and right are lists ( List(lhs_field) | ListView(lhs_field) | FixedSizeList(lhs_field, _), List(rhs_field) | ListView(rhs_field) | FixedSizeList(rhs_field, _), ) => Some(List(coerce_list_children(lhs_field, rhs_field)?)), _ => None, }

Is ListView already supported in arrow? I would prefer to handle list view if there is corresponding test as well to ensure the test coverage

TBH I'd as well would handle this separately with a proper test. I'll put a ticket

but will reorder the arms

Co-authored-by: Jay Zhan <[email protected]>

…coerce-arrays-inner-types

alamb · 2024-11-20T22:15:58Z

@jayzhan211 are you good to merge this PR?

jayzhan211 · 2024-11-21T00:05:04Z

Thanks @blaginin @alamb @findepi

Coerce Array inner types

5d109d1

findepi reviewed Nov 16, 2024

View reviewed changes

datafusion/expr-common/src/type_coercion/binary.rs Outdated Show resolved Hide resolved

datafusion/expr-common/src/type_coercion/binary.rs Outdated Show resolved Hide resolved

datafusion/expr-common/src/type_coercion/binary.rs Outdated Show resolved Hide resolved

blaginin added 4 commits November 16, 2024 17:41

coerce_list_children macro → fn

9ddeda1

Rearrange & simplify patterns

f353e3f

Add casting for FixedSizeList

22414e3

Add sql logic test

3c9a768

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Nov 16, 2024

Fix test

6e0c56b

blaginin marked this pull request as ready for review November 16, 2024 18:22

findepi reviewed Nov 16, 2024

View reviewed changes

datafusion/sqllogictest/test_files/union.slt Outdated Show resolved Hide resolved

Expand error

d169abc

findepi reviewed Nov 17, 2024

View reviewed changes

blaginin added 3 commits November 17, 2024 14:02

Switch to Arc::clone

baf99cb

OR nullable-s

639f236

Add successful test

2691f80

This comment was marked as outdated.

Sign in to view

jayzhan211 previously approved these changes Nov 18, 2024

View reviewed changes

blaginin requested a review from jayzhan211 November 18, 2024 19:37

alamb approved these changes Nov 19, 2024

View reviewed changes

blaginin added 6 commits November 19, 2024 16:35

Switch to type_union_resolution

1181abc

Add a note on coerce_list_children

306e7fa

Handle different list types inside type_union_resolution_coercion

18ee213

Add a make_array test with different array types

cb72340

Merge remote-tracking branch 'origin/coerce-arrays-inner-types' into …

4005c0c

…coerce-arrays-inner-types

Add the test value

91b468d

blaginin commented Nov 19, 2024

View reviewed changes

jayzhan211 reviewed Nov 20, 2024

View reviewed changes

datafusion/sqllogictest/test_files/array.slt Show resolved Hide resolved

findepi approved these changes Nov 20, 2024

View reviewed changes

blaginin and others added 3 commits November 20, 2024 17:38

Update datafusion/sqllogictest/test_files/array.slt

a3052be

Co-authored-by: Jay Zhan <[email protected]>

Reorder match

1692229

Merge remote-tracking branch 'origin/coerce-arrays-inner-types' into …

5d39c20

…coerce-arrays-inner-types

blaginin requested a review from jayzhan211 November 20, 2024 17:45

jayzhan211 merged commit 240402d into apache:main Nov 21, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coerce Array inner types #13452

Coerce Array inner types #13452

blaginin commented Nov 16, 2024

findepi left a comment

blaginin commented Nov 16, 2024

findepi Nov 17, 2024

blaginin Nov 17, 2024

jayzhan211 Nov 18, 2024 •

edited

Loading

blaginin Nov 18, 2024

jayzhan211 Nov 19, 2024

jayzhan211 Nov 19, 2024

findepi Nov 19, 2024

blaginin Nov 19, 2024

findepi Nov 17, 2024

findepi Nov 18, 2024

blaginin Nov 18, 2024

findepi Nov 19, 2024

alamb Nov 19, 2024

findepi Nov 20, 2024

This comment was marked as outdated.

jayzhan211 left a comment

alamb left a comment •

edited

Loading

alamb Nov 19, 2024

blaginin Nov 19, 2024 •

edited

Loading

jayzhan211 Nov 20, 2024

jayzhan211 Nov 20, 2024

findepi Nov 20, 2024

jayzhan211 Nov 20, 2024 •

edited

Loading

blaginin Nov 20, 2024

blaginin Nov 20, 2024

alamb commented Nov 20, 2024

jayzhan211 commented Nov 21, 2024

Coerce Array inner types #13452

Coerce Array inner types #13452

Conversation

blaginin commented Nov 16, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

findepi left a comment

Choose a reason for hiding this comment

blaginin commented Nov 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

jayzhan211 left a comment

Choose a reason for hiding this comment

alamb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaginin Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 20, 2024

jayzhan211 commented Nov 21, 2024

jayzhan211 Nov 18, 2024 •

edited

Loading

alamb left a comment •

edited

Loading

blaginin Nov 19, 2024 •

edited

Loading

jayzhan211 Nov 20, 2024 •

edited

Loading