alamb commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1420362453
##########
datafusion/expr/src/signature.rs:
##########
@@ -113,6 +115,8 @@ pub enum TypeSignature {
/// Function `make_array` takes 0 or more arguments with arbitrary types,
its `TypeSignature`
/// is `OneOf(vec![Any(0), VariadicAny])`.
OneOf(Vec<TypeSignature>),
+ /// Specialized Signature for ArrayAppend and similar functions
Review Comment:
What do you think about using a more generic name. Perhaps something like
```rust
/// The first argument is an array type ([`DataType::List`], or
[`DataType::LargeList`]
/// and the subsequent arguments are coerced to the List's element type
///
/// For example a call to `func(a: List(int64), b: int32, c: utf8)`
would attempt to coerce
/// all the arguments to `int64`:
/// ```
/// func(a: List(int64), cast(b as int64): int64, cast(c as int64):
int64)
/// ```
ArrayAndElements
```
There may be more general ways of expressing the array function types too 🤔
##########
datafusion/expr/src/signature.rs:
##########
@@ -95,6 +95,8 @@ pub enum TypeSignature {
VariadicEqual,
/// One or more arguments with arbitrary types
VariadicAny,
+ /// A function such as `make_array` should be coerced to the same type
+ VariadicCoerced,
Review Comment:
Can you explain how this is different than `VariadicEqual`? It seems from
the comments here they are quite similar 🤔
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -590,26 +590,6 @@ fn coerce_arguments_for_fun(
.collect::<Result<Vec<_>>>()?;
}
- if *fun == BuiltinScalarFunction::MakeArray {
Review Comment:
❤️
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -590,26 +590,6 @@ fn coerce_arguments_for_fun(
.collect::<Result<Vec<_>>>()?;
}
- if *fun == BuiltinScalarFunction::MakeArray {
Review Comment:
❤️
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -265,10 +265,8 @@ AS VALUES
(make_array([28, 29, 30], [31, 32, 33], [34, 35, 36], [28, 29, 30], [31, 32,
33], [34, 35, 36], [28, 29, 30], [31, 32, 33], [34, 35, 36], [28, 29, 30]),
[28, 29, 30], [37, 38, 39], 10)
;
-query ?
+query error
select [1, true, null]
Review Comment:
this is an error because `true` can't be coerced to an integer, right? FWIW
I think that is fine and is consistent with the postgres rues:
```
postgres=# select array[1, true, null];
ERROR: ARRAY types integer and boolean cannot be matched
LINE 1: select array[1, true, null];
```
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -1092,18 +1090,27 @@ select list_sort(make_array(1, 3, null, 5, NULL, -5)),
list_sort(make_array(1, 3
## array_append (aliases: `list_append`, `array_push_back`, `list_push_back`)
-# TODO: array_append with NULLs
-# array_append scalar function #1
-# query ?
-# select array_append(make_array(), 4);
-# ----
-# [4]
+# array_append with NULLs
-# array_append scalar function #2
-# query ??
-# select array_append(make_array(), make_array()), array_append(make_array(),
make_array(4));
-# ----
-# [[]] [[4]]
+query ???????
+select
+ array_append(null, 1),
Review Comment:
Do we want to support `array_append(null, ...`)?
Postgres does not allow this:
```
postgres=# select array_append(null, array[2,3]);
ERROR: could not find array type for data type integer[]
```
It also does't try to find a fancy type with an empty list:
```
postgres=# select array_append(array[], array[2,3]);
ERROR: cannot determine type of empty array
LINE 1: select array_append(array[], array[2,3]);
^
HINT: Explicitly cast to the desired type, for example ARRAY[]::integer[].
```
🤔
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -1092,18 +1090,27 @@ select list_sort(make_array(1, 3, null, 5, NULL, -5)),
list_sort(make_array(1, 3
## array_append (aliases: `list_append`, `array_push_back`, `list_push_back`)
-# TODO: array_append with NULLs
-# array_append scalar function #1
-# query ?
-# select array_append(make_array(), 4);
-# ----
-# [4]
+# array_append with NULLs
-# array_append scalar function #2
-# query ??
-# select array_append(make_array(), make_array()), array_append(make_array(),
make_array(4));
-# ----
-# [[]] [[4]]
+query ???????
+select
+ array_append(null, 1),
Review Comment:
Do we want to support `array_append(null, ...`)?
Postgres does not allow this:
```
postgres=# select array_append(null, array[2,3]);
ERROR: could not find array type for data type integer[]
```
It also does't try to find a fancy type with an empty list:
```
postgres=# select array_append(array[], array[2,3]);
ERROR: cannot determine type of empty array
LINE 1: select array_append(array[], array[2,3]);
^
HINT: Explicitly cast to the desired type, for example ARRAY[]::integer[].
```
🤔
##########
datafusion/expr/src/signature.rs:
##########
@@ -113,6 +115,8 @@ pub enum TypeSignature {
/// Function `make_array` takes 0 or more arguments with arbitrary types,
its `TypeSignature`
/// is `OneOf(vec![Any(0), VariadicAny])`.
OneOf(Vec<TypeSignature>),
+ /// Specialized Signature for ArrayAppend and similar functions
Review Comment:
What do you think about using a more generic name. Perhaps something like
```rust
/// The first argument is an array type ([`DataType::List`], or
[`DataType::LargeList`]
/// and the subsequent arguments are coerced to the List's element type
///
/// For example a call to `func(a: List(int64), b: int32, c: utf8)`
would attempt to coerce
/// all the arguments to `int64`:
/// ```
/// func(a: List(int64), cast(b as int64): int64, cast(c as int64):
int64)
/// ```
ArrayAndElements
```
There may be more general ways of expressing the array function types too 🤔
##########
datafusion/expr/src/signature.rs:
##########
@@ -95,6 +95,8 @@ pub enum TypeSignature {
VariadicEqual,
/// One or more arguments with arbitrary types
VariadicAny,
+ /// A function such as `make_array` should be coerced to the same type
+ VariadicCoerced,
Review Comment:
Can you explain how this is different than `VariadicEqual`? It seems from
the comments here they are quite similar 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
