feat: support FixedSizeList Type Coercion

[Weijun-H]

Which issue does this PR close?

Follow #8902

Rationale for this change

For future maintenance, extend Array which specialized Signature for ArrayEmpty and similar functions

What changes are included in this PR?

• The signatures of array functions with one parameter to use Signature::array

Are these changes tested?

Are there any user-facing changes?

[Weijun-H]

follow PostgreSQL

[alamb]

Thanks @Weijun-H -- this is looking good. I have a question about the inner functions and the need for allow_null_coercion

[alamb]

Could you expand this comment to explain what null type coercion is and when it would make sense notn to be allowed?

I don't understand what it is used for 🤔 it seems to me like null coercion should always be allowed

[alamb]

since we are passing allow_null_coercion here, maybe it would make sense to have allow_null_coercion to be part of each array enum? Perhaps something like

impl ArrayFunctionSignature {
    ArrayAndElement(allow_null_coercion),
    ...
    Array(allow_null_coercion)
}

But again I am not sure about the need for allow_null_coercion 🤔

[Weijun-H]

allow_null_coercion determines if the signature can allow a null type. For example, empty(NULL) accepts NULL while array_dims(Null) will return error

[alamb]

I see -- so one part of this PR is extracting this function into ArrayFunctionSignature ?

[alamb]

is there any reason to create these inner functions, rather than actual functions (or maybe just inline the body of the functions into the match statement below)?

I find the extra level of indenting somewhat confusing

[Weijun-H]

It is only for the match statement below

[jayzhan211]

why do we need allow_null_coercion?

[Weijun-H]

This parameter is to determine if the signature could accept NULL

[jayzhan211]

is there any array function's signature is array_and_element(true, )? If not, it is better not to introduce allow_null_coercion for array_and_element now.

[Weijun-H]

array_has , array_positions, array_remove and array_remove_allshould use it.

D SELECT list_contains(null, 1);
┌────────────────────────┐
│ list_contains(NULL, 1) │
│         int32          │
├────────────────────────┤
│                        │
└────────────────────────┘

D SELECT list_position(null, 1);
┌────────────────────────┐
│ list_position(NULL, 1) │
│         int32          │
├────────────────────────┤
│                        │
└────────────────────────┘

**Schema (PostgreSQL v15)**

---

**Query #1**

    SELECT array_remove(NULL, 2);

| array_remove |
| ------------ |
|              |

[jayzhan211]

I think we should return error if not List

[jayzhan211]

why do we need to handle null here (any other array functions)? Is it like pre-checking? If it is, note that array_element(null, ) should be an error.

[jayzhan211]

We may need functions like pre-checking, or validation for checking the input of the Array function when we move to UDF-based implementation 🤔 .

[jayzhan211]

I think handling null here makes it confusing where it is nulls handled, is it handled incoercion step coerce signature or here (array_expressions). The test in array.slt only process once. We can only know it is handled correctly, but unsure which one does the job.

[Weijun-H]

I think handling null here makes it confusing where it is nulls handled, is it handled incoercion step coerce signature or here (array_expressions). The test in array.slt only process once. We can only know it is handled correctly, but unsure which one does the job.

This is what I am considering now, in coercion step could filter if the function accepts the NULL type while in array_expressions it determines what the function should return @jayzhan211 .

[jayzhan211]

in coercion step could filter if the function accepts the NULL type while in array_expressions it determines what the function should return

Does it mean we still need to do the null handling in array_expression? Take array_append for example, if we find null, we convert it to i64(None) and process it later on.

ideally, nulls should be handled in type coercion. And, array_expression can be simple that just focuses on processing the result and not handling type or signature-related issues.

If doing the checking again in array_expression is just because it is a public function, I think we should come out with a way that the checking can be somewhat reused in both places. Before that, maybe we just leave the checking in coercion step.

[Weijun-H]

in coercion step could filter if the function accepts the NULL type while in array_expressions it determines what the function should return

Does it mean we still need to do the null handling in array_expression? Take array_append for example, if we find null, we convert it to i64(None) and process it later on.

ideally, nulls should be handled in type coercion. And, array_expression can be simple that just focuses on processing the result and not handling type or signature-related issues.

It would be ideal if we could handle null in one place. However, array functions have different results for NULL. For instance, array_append(null, 1) returns an error, whereas array_has(null, 1) returns null.

[jayzhan211]

I notice that in postgres, array_append(null, 1) return {1}, array_append(null, ARRAY[1]) is error. 😕

[jayzhan211]

Maybe we can ignore nulls as the first argument since we are not coercing them to any other type. Unlike, array_append(arr, null) as the second argument, in this case, we will convert to other primitives.

array() for fixed list to list type coercion only.

[jayzhan211]

I believe our primary focus within TypeCoercionRewriter should be strictly on type coercion. If encountering a null value that isn't being cast to another type, it should be disregarded.

[Weijun-H]

I believe our primary focus within TypeCoercionRewriter should be strictly on type coercion. If encountering a null value that isn't being cast to another type, it should be disregarded.

I cannot agree more with that. I should focus on the type coercion, and leave NULL aside now. So I will remove allow_null_coercion and don't check null now.

[jayzhan211]

I still think it can be further simplied if there is a function that process coerced_fixed_size_list_to_list and return array_type. Then can be used both in array_and_index and array

[jayzhan211]

Actually, I don't think we need to place all signature functions into get_type_signature.
Instead, having separate signature functions. And, collect what we need in the matching function.

This way the function can be reused easily. For example,

fn array() return array_type.
match {
 signature::array => { vec![vec![array()]] }
  signature::array_and_index => { vec![vec![array(), i64]]  }
}

[jayzhan211]

Suggested change

	fn array_and_index(current_types: &[DataType]) -> Result<Vec<Vec<DataType>>> {
	fn array(array_type: &DataType) -> Option<DataType> {
	match array_type {
	DataType::List(_)
	| DataType::LargeList(_)
	| DataType::FixedSizeList(_, _) => {
	let array_type = coerced_fixed_size_list_to_list(array_type);
	Some(array_type)
	}
	_ => None,
	}
	}
	match self {
	ArrayFunctionSignature::ArrayAndElement => {
	array_append_or_prepend_valid_types(current_types, true)
	}
	ArrayFunctionSignature::ElementAndArray => {
	array_append_or_prepend_valid_types(current_types, false)
	}
	ArrayFunctionSignature::ArrayAndIndex => {
	if current_types.len() != 2 {
	return Ok(vec![vec![]]);
	}
	let array_type = array(&current_types[0]);
	if let Some(array_type) = array_type {
	Ok(vec![vec![array_type, DataType::Int64]])
	} else {
	Ok(vec![])
	}
	}
	ArrayFunctionSignature::ArrayAndElementAndOptionalIndex => {
	array_element_and_optional_index(current_types)
	}
	ArrayFunctionSignature::Array => {
	if current_types.len() != 1 {
	return Ok(vec![vec![]]);
	}
	let array_type = array(&current_types[0]);
	if let Some(array_type) = array_type {
	Ok(vec![vec![array_type]])
	} else {
	Ok(vec![])
	}
	}
	}

[jayzhan211]

I prefer to return the data type for array instead of long but useless Result<Vec<Vec<DataType>>>

[jayzhan211]

why is this change?

[jayzhan211]

LGTM, thanks @Weijun-H

[jayzhan211]

👍

[jayzhan211]

@Weijun-H, thanks again!