Support `FixedSizeList` type coercion by Weijun-H · Pull Request #8902 · apache/datafusion

Weijun-H · 2024-01-18T06:57:37Z

Which issue does this PR close?

Rationale for this change

When the argument has a FixedSizeList, type coercion is applied to the array function. This pull request only affects the function that uses two arguments, which are array_element, array_has, array_positions, array_prepend, array_remove, array_remove_all , and array_has.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Weijun-H · 2024-01-18T06:59:52Z

~~Stalled by #8344~~

Weijun-H · 2024-01-21T02:51:45Z

+ BuiltinScalarFunction::ArrayPrepend => {
+ Signature::element_and_array(self.volatility())
+ }
 BuiltinScalarFunction::ArrayRepeat => Signature::any(2, self.volatility()),


Although the functions including array_repeat, array_union, array_intersect, and array_except are also two-argument, Signature::array_and_element could not handle NULL well for them. The following PR could continue to work on this issue.

jayzhan211 · 2024-01-22T03:43:15Z

 | BuiltinScalarFunction::ArrayHasAny
- | BuiltinScalarFunction::ArrayHas => Signature::any(2, self.volatility()),
+ | BuiltinScalarFunction::ArrayHas => {
+ Signature::array_and_element(self.volatility())


Can you add the example that works after this change.
I did not find one in array.slt
One of the example is array_has(list, null)

array_has_any and array_has_any cannot use array_and_element, because their signature is array and array.

jayzhan211 · 2024-01-22T03:51:21Z

 Utf8 | LargeUtf8 => Some(type_into.clone()),
 Null if can_cast_types(type_from, type_into) => Some(type_into.clone()),

+ List(_) if matches!(type_from, FixedSizeList(_, _)) => Some(type_into.clone()),


Will it be easier if we run Fixedsizelist to List like coerce_arguments_for_fun before this signature type coercion?

I am unable to decide between coerce_arguments_for_fun and coerce_arguments_for_signature. Are there specific benefits for each?

One is coerced based on function, another is based on signature. If we target to convert from fixedsizelist to list, it seems coerce_arguments_for_fun is more suitable.

I still think we can have fixed size list to list conversion in one place.
Currently, it converts both in here and coerce_arguments_for_fun, right? Is it possible to have it once?

move coerce_arguments_for_fun before signature? Convert FixedSizeList to List before signature coercion. Then we don't need to deal with FixedSizeList

Yes, we can.
In ExprSchemale(https://github.com/apache/arrow-datafusion/blob/a7cdf605a1e23608e51889988c239613c80fb671/datafusion/expr/src/expr_schema.rs#L91-L105)
It forces the function return type consistent, but I am not sure of its purpose. Maybe @2010YOUY01 could explain it more.

I think we dont need this anymore, input types should be handled later on via coerce_arguments_for_signature. They are doing the same things

You can move the error handling here
https://github.com/apache/arrow-datafusion/blob/f4fc2639f1d9d1f4dbc73d39990a83f6bf7a725f/datafusion/optimizer/src/analyzer/type_coercion.rs#L560

Weijun-H · 2024-01-26T12:27:10Z

+ /// Specifies Signatures for array functions
+ ArraySignature(ArrayFunctionSignature),


For future extension

I think that is a good change, though a change of the API

jayzhan211 · 2024-01-29T06:07:12Z

- // Cast Fixedsizelist to List for array functions
- if *fun == BuiltinScalarFunction::MakeArray {
+ // coerce the fixed size list to list for all array fucntions
+ if fun.name().contains("array") {


There are two problems that I think we should stick to the previous checking.
name() takes the first aliases of the function, so if there we forgot to have first aliases starts with array, it fails.
We just need to care about make_array only, then other functions should get non-fixedsizelsit as input.

Why do we only need to care about make_array? If we put coerce_arguments_for_fun in front of coerce_arguments_for_signature, it should cast all FixedSizeList to List regarding array functions.

Because any kind of array functions includes make_array(). Like array_append(cast(fixedsizelist, list), element). we dont need to check again with array_append.

Yes, but it is difficult for FixedSizeList type coercion. My idea is to do these staff in coerce_arguments_for_signature based on different ArraySignature(ArrayFunctionSignature), which will benefit future maintenance and development. WDYT?

Ok, I know the reason, it seems we still need to handle fixedsizelist for other array functions because we can pass Fixedsizelist directly to them with arrow_cast.
In this case, I agree we should handle them in coerce_arguments_for_signature. And, coerce_arguments_for_fun can be removed.

We can keep coerce_arguments_for_fun until all FixedSizeList type coercion work is done.

…ay argument

jayzhan211

LGTM

Weijun-H · 2024-01-30T05:18:26Z

@jayzhan211 Thank you for your review

alamb · 2024-01-30T21:33:31Z

Thank you @Weijun-H and @jayzhan211 -- I plan to review this tomorrow if possible. I know it has been outstanding for quite a while

alamb

Thank you @Weijun-H -- really nice

Also, thank you @jayzhan211 for the review. It was wonderful to review this PR after you and @Weijun-H have worked on it and find the structure in such good shape

alamb · 2024-01-31T22:14:11Z

+ /// Specifies Signatures for array functions
+ ArraySignature(ArrayFunctionSignature),


I think that is a good change, though a change of the API

alamb · 2024-01-31T22:14:51Z

 volatility,
 }
 }
+ /// Specialized Signature for ArrayAppend and similar functions


Nice cleanup

alamb · 2024-01-31T22:19:16Z

 FROM arrays
 ;

+#TODO: create FixedSizeList with NULL column


Shall we maybe track this TODO with a separate ticket?

Tracked by #9094

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2024-02-01T16:42:46Z

Thanks again @Weijun-H !

github-actionsBot added sql SQL Planner logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Jan 18, 2024

Weijun-H changed the title ~~Minor: Add support for parsing FixedSizeList type~~Minor: support FixedSizeList type coercionJan 18, 2024

jayzhan211 mentioned this pull request Jan 18, 2024
Support FixedSizeListArray for array #8249
Closed
16 tasks

Weijun-H force-pushed the fixedsizelist-type-coercion branch from f169af2 to ac81670Compare January 20, 2024 02:01

github-actionsBot removed the sql SQL Planner label Jan 20, 2024

Weijun-H mentioned this pull request Jan 20, 2024
Improve array_concat signature for null and empty array #8594
Closed

Weijun-H commented Jan 21, 2024
View reviewed changes

Weijun-H marked this pull request as ready for review January 21, 2024 02:51

alamb mentioned this pull request Jan 21, 2024
DataFusion weekly project plan (Andrew Lamb) - Jan 22, 2024 #8933
Closed
9 tasks

jayzhan211 reviewed Jan 22, 2024
View reviewed changes

Comment threaddatafusion/expr/src/type_coercion/functions.rs Outdated

jayzhan211 reviewed Jan 22, 2024
View reviewed changes

Comment threaddatafusion/common/src/utils.rs

jayzhan211 reviewed Jan 22, 2024
View reviewed changes

Comment threaddatafusion/expr/src/type_coercion/functions.rs Outdated

jayzhan211 reviewed Jan 22, 2024
View reviewed changes

Comment threaddatafusion/expr/src/type_coercion/functions.rs Outdated

Weijun-H marked this pull request as draft January 24, 2024 04:19

github-actionsBot added the optimizer Optimizer rules label Jan 24, 2024

Weijun-H marked this pull request as ready for review January 24, 2024 11:48

Weijun-H requested a review from jayzhan211 January 24, 2024 11:49

jayzhan211 reviewed Jan 26, 2024
View reviewed changes

Comment threaddatafusion/expr/src/type_coercion/functions.rs Outdated

jayzhan211 reviewed Jan 26, 2024
View reviewed changes

Comment threaddatafusion/expr/src/built_in_function.rs Outdated

Weijun-H force-pushed the fixedsizelist-type-coercion branch from e1d8809 to 2d16386Compare January 26, 2024 12:25

Weijun-H commented Jan 26, 2024
View reviewed changes

Weijun-H requested a review from jayzhan211 January 27, 2024 08:35

alamb mentioned this pull request Jan 28, 2024
DataFusion weekly project plan (Andrew Lamb) - Jan 29, 2024 #9030
Closed
6 tasks

jayzhan211 reviewed Jan 29, 2024
View reviewed changes

Add support for parsing FixedSizeList type
4a40883

Weijun-H added 18 commits January 30, 2024 12:04

support fixedsizelist for functinos with array-element or element-arr…
bb20c37
…ay argument

add tests for array_element
53bf53a

add tests for array_remove
c7cd1f5

add tests for array_remove_n
b9474ec

add tests for array_has
597c262

fix comment
97dc372

fix comment
b01cca0

add tests for array_positoins
2ecb193

test chore
7e0573b

test chore
25e620a

remove useless logic
0c08cc9

refatctor coerced_type_with_base_type_only function
19e322a

add null test for array_has
707c0c8

refactor: put fixedsizelist in coerce_arguments_for_fun
acf9df4

chore
9c82e43

refactor type signature
159648a

add array_and_index function
76cede4

put all type coercion in coerce_arguments_for_signature
b1d79ba

Weijun-H force-pushed the fixedsizelist-type-coercion branch from 24bc9ea to b1d79baCompare January 30, 2024 04:04

jayzhan211 approved these changes Jan 30, 2024
View reviewed changes

alamb changed the title ~~Minor: support FixedSizeList type coercion~~Support FixedSizeList type coercionJan 31, 2024

alamb added the api change Changes the API exposed to users of the crate label Jan 31, 2024

alamb approved these changes Jan 31, 2024
View reviewed changes

add comment
832b59a
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb merged commit 038763c into apache:mainFeb 1, 2024

alamb mentioned this pull request Feb 1, 2024
Incorrect column cast when coalescing array type #9098
Closed

Weijun-H mentioned this pull request Feb 2, 2024
feat: support FixedSizeList Type Coercion #9108
Merged

		/// Specifies Signatures for array functions
		ArraySignature(ArrayFunctionSignature),

Uh oh!

Conversation

Weijun-H commented Jan 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Weijun-H commented Jan 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211Jan 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Weijun-HJan 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Weijun-HJan 28, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211Jan 29, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211Jan 29, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

Weijun-H commented Jan 30, 2024

Uh oh!

alamb commented Jan 30, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Weijun-H commented Jan 18, 2024•
edited
Loading

Weijun-H commented Jan 18, 2024•
edited
Loading

jayzhan211Jan 22, 2024•
edited
Loading

Weijun-HJan 22, 2024•
edited
Loading

Weijun-HJan 28, 2024•
edited
Loading

jayzhan211Jan 29, 2024•
edited
Loading

jayzhan211Jan 29, 2024•
edited
Loading