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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
