You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jayzhan211 (via GitHub)" <gi...@apache.org> on 2023/11/25 06:26:48 UTC
[PR] Move Coercion for MakeArray while verifying signature and introduce signature verification for ArrayAppend [arrow-datafusion]
jayzhan211 opened a new pull request, #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Ref #7142
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Fix Null Handing for array function. ArrayAppend first, others later.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Move coercion for MakeArray to different place.
Add signature for ArrayAppend
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
MakeArray - existing test
ArrayAppend - new test
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#issuecomment-1861386570
I merged this branch up from main to ensure there are no logical conflicts, and if all tests pass I intend to merge it
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1421200843
##########
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:
Summary of other sql behavior
Duckdb: [[2, 3]], [[2, 3]]
ClickHouse: null, [[2, 3]]
Postgres: error, error
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1421179330
##########
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:
I just noticed that `VariadicEqual` is not used in any function. Maybe we can just keep one.
I thought Equal is the one that don't care about coercion. All the type should be equal like (i32, i32, i32).
Coerced is the one that ensuring the final coerced type is the same (all of the type coercible to the same one), like (i32, i64) -> i64.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1421202307
##########
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:
I think for array_append([], [2,3]), it is fine to not follow postgres and return [[2, 3]] like clickhouse and duckdb.
For array_append(null, [2, 3]), I think we can follow postrgres.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1421181421
##########
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:
I think current `VariadicCoerced` includes `VariadicEqual` use case as well. We can just have one signature
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1425330025
##########
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:
yes
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#issuecomment-1846263452
@alamb Ready for review! I hope this is a better solution for dealing with nulls. I had tried `Trait SignatureComputation`, but it is a little overly complex.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1404759553
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -181,7 +239,7 @@ fn coerced_from<'a>(
Int64
if matches!(
type_from,
- Null | Int8 | Int16 | Int32 | Int64 | UInt8 | UInt16 | UInt32
+ Null | Int8 | Int16 | Int32 | Int64 | UInt8 | UInt16 | UInt32 | Boolean
Review Comment:
Only i64 is fix for passing existing test in array.slt. More type should be fixed in https://github.com/apache/arrow-datafusion/issues/8302
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -181,7 +239,7 @@ fn coerced_from<'a>(
Int64
if matches!(
type_from,
- Null | Int8 | Int16 | Int32 | Int64 | UInt8 | UInt16 | UInt32
+ Null | Int8 | Int16 | Int32 | Int64 | UInt8 | UInt16 | UInt32 | Boolean
Review Comment:
Only i64 is fixrf for passing existing test in array.slt. More type should be fixed in https://github.com/apache/arrow-datafusion/issues/8302
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#issuecomment-1829831277
wait on #8318 and #8331
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1421205568
##########
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:
Clickhouse and Duckdb has the same output for `array_append(make_array(null, null), 1) ` too.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1413281259
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -85,6 +91,24 @@ fn get_valid_types(
.iter()
.map(|valid_type| (0..*number).map(|_| valid_type.clone()).collect())
.collect(),
+ TypeSignature::VariadicCoerced => {
+ let new_type = current_types.iter().skip(1).try_fold(
+ current_types.first().unwrap().clone(),
+ |acc, x| {
+ let corced_type = comparison_coercion(&acc, x);
+ if let Some(corced_type) = corced_type {
+ Ok(corced_type)
+ } else {
+ internal_err!("Coercion from {acc:?} to {x:?} failed.")
Review Comment:
We call unwrap_or previously so `select [1, true, null]` unexpectedly correct since true is castable to 1 in arrow-rs but not in datafusion. `select [true, 1, null]` failed. It is better that we just return error if not coercible.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1404759206
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -95,7 +110,50 @@ fn get_valid_types(
TypeSignature::VariadicAny => {
vec![current_types.to_vec()]
}
+
TypeSignature::Exact(valid_types) => vec![valid_types.clone()],
+ TypeSignature::ArrayAppendLikeSignature => {
Review Comment:
Have a specialized checking for array append at the end. And I think we will need specialized check for other pattern too
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8317:
URL: https://github.com/apache/arrow-datafusion/pull/8317#discussion_r1404759553
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -181,7 +239,7 @@ fn coerced_from<'a>(
Int64
if matches!(
type_from,
- Null | Int8 | Int16 | Int32 | Int64 | UInt8 | UInt16 | UInt32
+ Null | Int8 | Int16 | Int32 | Int64 | UInt8 | UInt16 | UInt32 | Boolean
Review Comment:
Only i64 is fixed for passing existing test in array.slt. More types should be fixed in https://github.com/apache/arrow-datafusion/issues/8302
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org