You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alexanderbianchi (via GitHub)" <gi...@apache.org> on 2025/06/09 21:13:35 UTC
[PR] Fix array_concat with NULL arrays [datafusion]
alexanderbianchi opened a new pull request, #16348:
URL: https://github.com/apache/datafusion/pull/16348
Fixes issue where 'select array_concat(NULL::integer[])' would throw 'Arrow error: Compute error: concat requires input of at least one array'
## Which issue does this PR close?
- Closes #.
## What changes are included in this PR?
- Fix array_concat_inner to properly handle NULL list arrays by checking null_count
- Add logic to create properly typed empty arrays for all-null inputs
- Support NULL handling for List, LargeList, and FixedSizeList types
- Add comprehensive test coverage for NULL array scenarios
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix array_concat with NULL arrays [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #16348:
URL: https://github.com/apache/datafusion/pull/16348#discussion_r2136979543
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -3070,6 +3070,30 @@ select array_concat([]);
----
[]
+# test with NULL array
+query ?
+select array_concat(NULL::integer[]);
+----
+NULL
+
Review Comment:
How about adding a couple of cases that test the other different list types?
```sql
# test with NULL LargeList
query ?
select array_concat(arrow_cast(NULL::string[], 'LargeList(Utf8)'));
----
NULL
# test with NULL FixedSizeList
query ?
select array_concat(arrow_cast(NULL::string[], 'FixedSizeList(2, Utf8)'));
----
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix array_concat with NULL arrays [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #16348:
URL: https://github.com/apache/datafusion/pull/16348#discussion_r2136961999
##########
datafusion/functions-nested/src/concat.rs:
##########
@@ -361,15 +362,44 @@ pub(crate) fn array_concat_inner(args: &[ArrayRef]) -> Result<ArrayRef> {
for arg in args {
match arg.data_type() {
DataType::Null => continue,
- DataType::LargeList(_) => large_list = true,
- _ => (),
+ DataType::LargeList(_) => {
+ large_list = true;
+ // Check if this large list array has any non-null rows
+ if arg.null_count() < arg.len() {
+ all_null = false;
+ }
+ }
+ DataType::List(_) | DataType::FixedSizeList(..) => {
+ // Check if this list array has any non-null rows
+ if arg.null_count() < arg.len() {
+ all_null = false;
+ }
+ }
+ _ => {
+ all_null = false;
+ }
Review Comment:
Nice, do you think the null count check could be performed always regarding of the type? something like:
```rust
match arg.data_type() {
DataType::Null => continue,
DataType::LargeList(_) => large_list = true,
_ => (),
}
+ if arg.null_count() < arg.len() {
+ all_null = false;
+ }
```
?
##########
datafusion/functions-nested/src/concat.rs:
##########
@@ -361,15 +362,44 @@ pub(crate) fn array_concat_inner(args: &[ArrayRef]) -> Result<ArrayRef> {
for arg in args {
match arg.data_type() {
DataType::Null => continue,
- DataType::LargeList(_) => large_list = true,
- _ => (),
+ DataType::LargeList(_) => {
+ large_list = true;
+ // Check if this large list array has any non-null rows
+ if arg.null_count() < arg.len() {
+ all_null = false;
+ }
+ }
+ DataType::List(_) | DataType::FixedSizeList(..) => {
+ // Check if this list array has any non-null rows
+ if arg.null_count() < arg.len() {
+ all_null = false;
+ }
+ }
+ _ => {
+ all_null = false;
+ }
}
-
- all_null = false
}
if all_null {
- Ok(Arc::new(NullArray::new(args[0].len())))
+ // Return a null array with the same type as the first non-null-type argument
+ let first_non_null_type = args
+ .iter()
+ .map(|arg| arg.data_type())
+ .find_or_first(|d| !d.is_null());
+
+ match first_non_null_type {
+ Some(DataType::List(field)) => Ok(Arc::new(
+ GenericListArray::<i32>::new_null(field.clone(), args[0].len()),
+ )),
+ Some(DataType::LargeList(field)) => Ok(Arc::new(
+ GenericListArray::<i64>::new_null(field.clone(), args[0].len()),
+ )),
+ Some(DataType::FixedSizeList(field, size)) => Ok(Arc::new(
+ FixedSizeListArray::new_null(field.clone(), *size, args[0].len()),
+ )),
+ _ => Ok(Arc::new(NullArray::new(args[0].len()))),
+ }
Review Comment:
There's this `ArrayData::new_null()` method that has already a match statement similar to this, maybe it can save some lines, something like:
```rust
if all_null {
+ Ok(arrow::array::make_array(ArrayData::new_null(
+ args[0].data_type(),
+ args[0].len(),
+ )))
} else if large_list {
```
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -3070,6 +3070,30 @@ select array_concat([]);
----
[]
+# test with NULL array
+query ?
+select array_concat(NULL::integer[]);
+----
+NULL
+
Review Comment:
How about adding a couple of cases that test the other different list tyles?
```sql
# test with NULL LargeList
query ?
select array_concat(arrow_cast(NULL::string[], 'LargeList(Utf8)'));
----
NULL
# test with NULL FixedSizeList
query ?
select array_concat(arrow_cast(NULL::string[], 'FixedSizeList(2, Utf8)'));
----
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix array_concat with NULL arrays [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #16348:
URL: https://github.com/apache/datafusion/pull/16348
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org