You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "izveigor (via GitHub)" <gi...@apache.org> on 2023/07/18 09:20:54 UTC
[GitHub] [arrow-datafusion] izveigor opened a new pull request, #7008: fix: `array_concat` with arrays with different dimensions
izveigor opened a new pull request, #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008
# 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.
-->
Closes https://github.com/apache/arrow-datafusion/issues/6992
Follow on to https://github.com/apache/arrow-datafusion/pull/6879
# 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.
-->
# 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.
-->
# Are these changes tested?
Yes
<!--
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)?
-->
# Are there any user-facing changes?
Yes
<!--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions, add `_list*` aliases for `_array*` functions
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1269793463
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -163,13 +163,20 @@ fn compute_array_length(
/// Returns the dimension of the array
fn compute_array_ndims(arr: Option<ArrayRef>) -> Result<Option<u64>> {
+ Ok(compute_array_ndims_with_datatype(arr)?.0)
+}
+
+/// Returns the dimension and lower datatype of the array
Review Comment:
```suggestion
/// Returns the dimension and the datatype of elements of the array
```
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -430,7 +430,22 @@ impl BuiltinScalarFunction {
)
}
- /// Returns the output [`DataType` of this function
+ /// Returns the dimension [`DataType`] of [`DataType::List`]
Review Comment:
```suggestion
/// Returns the dimension [`DataType`] of [`DataType::List`].
///
/// Dimension is defined as the deepest level of nesting.
/// * `Int64` has dimension 1
/// * `List(Int64)` has dimension 2
/// * `List(List(Int64))` has dimension 3
/// * etc.
```
--
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
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1267878467
##########
datafusion/core/tests/sqllogictests/test_files/array.slt:
##########
@@ -149,7 +149,7 @@ select column1, column2, column3 from arrays_values_without_nulls;
### Array function tests
-## make_array
+## make_array (aliases: `make_list`)
Review Comment:
Any reason to have the alias `make_list`, may it be confused with creating ListArray?
--
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
[GitHub] [arrow-datafusion] izveigor commented on pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#issuecomment-1641903629
@alamb @jayzhan211 I wonder if you have free time to review the PR.
--
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
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1268001289
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
Review Comment:
I think max_dims should be 0, If all the list is 1D, we will get null for expr_type.
--
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
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1268011928
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
for input_expr_type in input_expr_types {
match input_expr_type {
List(field) => {
if !field.data_type().equals_datatype(&Null) {
Review Comment:
I think this if-statement is not necessary.
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
Review Comment:
If the init value is 1 and all the lists are 1D, we will get null for expr_type, right? But why doesn't the 1D concat test fails?
init value for expr_type = null, and max_dims = 0, with updating expr_type with input_expr_type instead of field since much more straightforward to me.
--
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
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1268172363
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
Review Comment:
Implemented your idea 👌.
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
Review Comment:
Implemented your idea 👌.
--
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
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1267924137
##########
datafusion/core/tests/sqllogictests/test_files/array.slt:
##########
@@ -149,7 +149,7 @@ select column1, column2, column3 from arrays_values_without_nulls;
### Array function tests
-## make_array
+## make_array (aliases: `make_list`)
Review Comment:
@jayzhan211 I have identified several reasons:
1) The idea was taken from popular database: DuckDB. So, in my opinion, users of this database can more easily adapt to our array functions. (Documentation: https://duckdb.org/docs/sql/functions/nested).
2) `array` and `list` prefix or postfix can separate mutable or immutable objects. (See discussion: https://github.com/apache/arrow-datafusion/discussions/6855 (About aliases)).
3) Some people prefer to use one prefix/postfix over another.
--
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
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1268028734
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
for input_expr_type in input_expr_types {
match input_expr_type {
List(field) => {
if !field.data_type().equals_datatype(&Null) {
Review Comment:
Is it possible to get a nested list array with a null type? i.e. List(List(Null))
--
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
[GitHub] [arrow-datafusion] alamb merged pull request #7008: fix: `array_concat` with arrays with different dimensions, add `_list*` aliases for `_array*` functions
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008
--
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
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1268011928
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
for input_expr_type in input_expr_types {
match input_expr_type {
List(field) => {
if !field.data_type().equals_datatype(&Null) {
Review Comment:
nit: I think this if-statement is not necessary.
--
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
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1268169437
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
for input_expr_type in input_expr_types {
match input_expr_type {
List(field) => {
if !field.data_type().equals_datatype(&Null) {
Review Comment:
Good catch! There is a bug https://github.com/apache/arrow-datafusion/issues/7028. Thanks, @jayzhan211!
--
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
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1267926905
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
for input_expr_type in input_expr_types {
match input_expr_type {
List(field) => {
if !field.data_type().equals_datatype(&Null) {
- expr_type = field.data_type().clone();
- break;
+ let dims = self.return_dimension(input_expr_type.clone());
Review Comment:
This is the main codebase, other changes are documentation improvements and new aliases.
--
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
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1267926692
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -430,7 +430,22 @@ impl BuiltinScalarFunction {
)
}
- /// Returns the output [`DataType` of this function
+ /// Returns the dimension [`DataType`] of [`DataType::List`]
+ fn return_dimension(self, input_expr_type: DataType) -> u64 {
Review Comment:
This is the main codebase, other changes are documentation improvements and new aliases.
--
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
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1268011928
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
for input_expr_type in input_expr_types {
match input_expr_type {
List(field) => {
if !field.data_type().equals_datatype(&Null) {
Review Comment:
nit: I think this if-statement is not necessary.
--
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
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1268001289
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
},
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
+ let mut max_dims = 1;
Review Comment:
I think max_dims should be 0, If all the list is 1D, we will get null for expr_type.
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7008: fix: `array_concat` with arrays with different dimensions, add `_list*` aliases for `_array*` functions
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#discussion_r1269874081
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -430,7 +430,22 @@ impl BuiltinScalarFunction {
)
}
- /// Returns the output [`DataType` of this function
+ /// Returns the dimension [`DataType`] of [`DataType::List`]
Review Comment:
Proposed PR to add this documentation: https://github.com/apache/arrow-datafusion/pull/7045
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #7008: fix: `array_concat` with arrays with different dimensions
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7008:
URL: https://github.com/apache/arrow-datafusion/pull/7008#issuecomment-1642756097
I will plan to review this PR tomorrow -- thank you @izveigor
--
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