Feat: Support array flatten() on `List(LargeList(_))` types #18363

sdf-jkl · 2025-10-29T19:27:57Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Added a flatten() List(LargeList) test to the sqllogictest

Added support for array flatten() on List(LargeList(_)) types

Are these changes tested?

sqllogictest passes, but I still need to implement a test where offsets could not be downcasted from i64 to i32

Are there any user-facing changes?

Users will be able to use flatten on List(LargeList) types

sdf-jkl

This currently is not a finished solution.

Still need to figure out how to create a testcase where offsets would not be able to downcast to i32.

Should it be a sqllogictest test using an array with i32::MAX + 1 values or something else?

sdf-jkl · 2025-10-29T19:35:23Z

datafusion/functions-nested/src/flatten.rs

+                List(field) | LargeList(field) | FixedSizeList(field, _) => {
+                    List(Arc::clone(field))


Currently this only supports arrays that can be converted from LargeList to List.

sdf-jkl · 2025-10-29T19:37:25Z

datafusion/functions-nested/src/flatten.rs

+                    let (inner_field, inner_offsets, inner_values, _) =
+                        as_large_list_array(&values)?.clone().into_parts();
+                    // Try to downcast the inner offsets to i32
+                    match downcast_i64_inner_to_i32(&inner_offsets, &offsets) {
+                        Ok(i32offsets) => {
+                            let flattened_array = GenericListArray::<i32>::new(
+                                inner_field,
+                                i32offsets,
+                                inner_values,
+                                nulls,
+                            );
+                            Ok(Arc::new(flattened_array) as ArrayRef)
+                        }
+                        // If downcast fails we keep the offsets as is
+                        Err(_) => {
+                            // Fallback: keep i64 offsets → LargeList<i64>
+                            let i64offsets = keep_offsets_i64(inner_offsets, offsets);
+                            let flattened_array = GenericListArray::<i64>::new(
+                                inner_field,
+                                i64offsets,
+                                inner_values,
+                                nulls,
+                            );
+                            Ok(Arc::new(flattened_array) as ArrayRef)


Here we are trying to downcast the inner array offsets from i64 to i32 and if fail we fallback to i64 offsets.
The fallback is not yet supported by the return_type

This doesn't make sense to me as it means the return value of the function might not conform to the stated return_type

If return type says the return array will be ListArray for a LargeListArray input, then the invoke function must return that value (or an error)

SO I think we need to either update the implementation and return typetype to return LargeListArray for LargeListArray inputs or else return an error if we can't cast the offsets correctly

I was looking at @Jefffrey's original comment suggesting that we should try to downcast it i32 returning a ListArray and alternatively return a LargeListArray if downcast is not possible.

He also mentioned that upcasting the parent offsets to LargeListArray blindly would be inefficient and undesirable.

I wanted to see if it's possible to infer the possibility of downcasting the inner array offsets within the return_type function. Then return the matching output type.

We could do it by checking if: inner_offsets.last() <= i32::MAX

Yep I did mention that, however I did make a note that:

though this might be tricky considering return_type() wouldn't know this until execution

I think we make a choice, either always upcast to large if one of the inner lists is a large list, or just error.

I guess the former is more favourable/robust than the latter.

I see, if the consensus is to always upcast to LargeListArray I'll proceed with it.

sdf-jkl · 2025-10-29T19:38:40Z

datafusion/functions-nested/src/flatten.rs

                }
                LargeList(_) => {
-                    let (inner_field, inner_offsets, inner_values, nulls) =
+                    let (inner_field, inner_offsets, inner_values, _) = // _ instead of nulls?


Here flattened_array was generated using the inner nulls instead of the outer ones. I figured it should follow the same format and use nulls from the outer array.

This is interesting; so this might be a bug in current behaviour? Do you think you can mock up a test where it fails on main without this fix?

sdf-jkl · 2025-10-29T19:39:18Z

datafusion/functions-nested/src/flatten.rs

+    inner_offsets: OffsetBuffer<O>,
+    outer_offsets: OffsetBuffer<O>,


A little naming change because the previous one was confusing.

sdf-jkl · 2025-10-29T19:43:46Z

datafusion/functions-nested/src/flatten.rs

+// Function for converting LargeList offsets into List offsets
+fn downcast_i64_inner_to_i32(
+    inner_offsets: &OffsetBuffer<i64>,
+    outer_offsets: &OffsetBuffer<i32>,
+) -> Result<OffsetBuffer<i32>, ArrowError> {
+    let buffer = inner_offsets.clone().into_inner();
+    let offsets: Result<Vec<i32>, _> = outer_offsets
+        .iter()
+        .map(|i| buffer[i.to_usize().unwrap()])
+        .map(|i| {
+            i32::try_from(i)
+                .map_err(|_| ArrowError::CastError(format!("Cannot downcast offset {i}")))
+        })
+        .collect();
+    Ok(OffsetBuffer::new(offsets?.into()))
+}


This function is trying to downcast offsets to i32.

On success it returns an OffsetBuffer<i32>

If it fails it errors out.

sdf-jkl · 2025-10-29T19:46:08Z

datafusion/functions-nested/src/flatten.rs

+}
+
+// In case the conversion fails we convert the outer offsets into i64
+fn keep_offsets_i64(


This function takes the outer i32 and inner i64 offsets and keeps the inner i64

I think this function should be renamed (and docstring adjusted) now? keep_offsets_i64 is a bit confusing to read for me

alamb

thanks for this @sdf-jkl

alamb · 2025-10-29T20:26:00Z

datafusion/functions-nested/src/flatten.rs

+                    let (inner_field, inner_offsets, inner_values, _) =
+                        as_large_list_array(&values)?.clone().into_parts();
+                    // Try to downcast the inner offsets to i32
+                    match downcast_i64_inner_to_i32(&inner_offsets, &offsets) {
+                        Ok(i32offsets) => {
+                            let flattened_array = GenericListArray::<i32>::new(
+                                inner_field,
+                                i32offsets,
+                                inner_values,
+                                nulls,
+                            );
+                            Ok(Arc::new(flattened_array) as ArrayRef)
+                        }
+                        // If downcast fails we keep the offsets as is
+                        Err(_) => {
+                            // Fallback: keep i64 offsets → LargeList<i64>
+                            let i64offsets = keep_offsets_i64(inner_offsets, offsets);
+                            let flattened_array = GenericListArray::<i64>::new(
+                                inner_field,
+                                i64offsets,
+                                inner_values,
+                                nulls,
+                            );
+                            Ok(Arc::new(flattened_array) as ArrayRef)


This doesn't make sense to me as it means the return value of the function might not conform to the stated return_type

If return type says the return array will be ListArray for a LargeListArray input, then the invoke function must return that value (or an error)

SO I think we need to either update the implementation and return typetype to return LargeListArray for LargeListArray inputs or else return an error if we can't cast the offsets correctly

…f-jkl/datafusion into flatten-listLargeList-support

Jefffrey · 2025-10-31T02:45:07Z

datafusion/functions-nested/src/flatten.rs

+}
+
+// In case the conversion fails we convert the outer offsets into i64
+fn keep_offsets_i64(


I think this function should be renamed (and docstring adjusted) now? keep_offsets_i64 is a bit confusing to read for me

Jefffrey · 2025-10-31T02:45:50Z

datafusion/sqllogictest/test_files/array.slt

+select flatten(arrow_cast(make_array([1], [2, 3], [null], make_array(4, null, 5)), 'FixedSizeList(4, LargeList(Int64))')),
+       flatten(arrow_cast(make_array([[1.1], [2.2]], [[3.3], [4.4]]), 'List(LargeList(FixedSizeList(1, Float64)))'));
+----
+[1, 2, 3, NULL, 4, NULL, 5] [[1.1], [2.2], [3.3], [4.4]]


I think should also add a check for the output type via arrow_typeof() to ensure we are indeed getting a large list

Jefffrey · 2025-10-31T02:46:56Z

datafusion/functions-nested/src/flatten.rs

                }
                LargeList(_) => {
-                    let (inner_field, inner_offsets, inner_values, nulls) =
+                    let (inner_field, inner_offsets, inner_values, _) = // _ instead of nulls?


This is interesting; so this might be a bug in current behaviour? Do you think you can mock up a test where it fails on main without this fix?

sdf-jkl · 2025-10-31T18:43:32Z

Thanks for your review @Jefffrey, I've addressed the issues you pointed out.

I'll check the potential bug in main

sdf-jkl · 2025-10-31T20:10:04Z

I made a test that points out the bug on a different branch, should I create a separate issue and a PR for it?

…f-jkl/datafusion into flatten-listLargeList-support

sdf-jkl · 2025-11-01T16:04:03Z

@Jefffrey @alamb Should be good now.

Jefffrey · 2025-11-02T03:27:19Z

Looks like some tests are failing in CI

…f-jkl/datafusion into flatten-listLargeList-support

sdf-jkl · 2025-11-02T18:39:41Z

@Jefffrey Fixed.

Jefffrey

LGTM

alamb · 2025-11-03T20:45:59Z

Thank you @sdf-jkl and @Jefffrey

sdf-jkl added 2 commits October 29, 2025 15:09

Add test to sqllogictest

e1166de

Add List(LargeList) support

ac2fac4

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Oct 29, 2025

sdf-jkl commented Oct 29, 2025

View reviewed changes

Merge branch 'main' into flatten-listLargeList-support

0ddf926

alamb reviewed Oct 29, 2025

View reviewed changes

sdf-jkl added 2 commits October 30, 2025 14:50

Update flatten to only upcast to LargeList

85bd164

Merge branch 'flatten-listLargeList-support' of https://github.com/sd…

37ce386

…f-jkl/datafusion into flatten-listLargeList-support

sdf-jkl requested a review from alamb October 30, 2025 20:03

Jefffrey reviewed Oct 31, 2025

View reviewed changes

sdf-jkl and others added 2 commits October 31, 2025 14:41

Address review comments

cf6ade3

Merge branch 'main' into flatten-listLargeList-support

0057356

This was referenced Oct 31, 2025

Flatten using the wrong validity buffer for LargeList(LargeList) #18419

Closed

Flatten using the wrong validity buffer for LargeList(LargeList) #18420

Closed

sdf-jkl added 3 commits November 1, 2025 11:58

Fix test to point out a bug

59f3fb8

Remove comment from fix

26cdd4e

Merge branch 'flatten-listLargeList-support' of https://github.com/sd…

659b42a

…f-jkl/datafusion into flatten-listLargeList-support

Merge branch 'main' into flatten-listLargeList-support

468ca6b

Jefffrey approved these changes Nov 2, 2025

View reviewed changes

sdf-jkl and others added 3 commits November 2, 2025 13:36

Fix expected test output

b77497e

Merge branch 'flatten-listLargeList-support' of https://github.com/sd…

fc4cc61

…f-jkl/datafusion into flatten-listLargeList-support

Merge branch 'main' into flatten-listLargeList-support

faf1c15

sdf-jkl requested a review from Jefffrey November 2, 2025 21:37

Jefffrey approved these changes Nov 3, 2025

View reviewed changes

alamb added this pull request to the merge queue Nov 3, 2025

Merged via the queue into apache:main with commit 0a0ccb1 Nov 3, 2025
28 checks passed

sdf-jkl deleted the flatten-listLargeList-support branch November 3, 2025 21:41

		List(field) \| LargeList(field) \| FixedSizeList(field, _) => {
		List(Arc::clone(field))

		inner_offsets: OffsetBuffer<O>,
		outer_offsets: OffsetBuffer<O>,

Feat: Support array flatten() on List(LargeList(_)) types #18363

Feat: Support array flatten() on List(LargeList(_)) types #18363

Uh oh!

Conversation

sdf-jkl commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

sdf-jkl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdf-jkl commented Oct 31, 2025

Uh oh!

sdf-jkl commented Oct 31, 2025

Uh oh!

sdf-jkl commented Nov 1, 2025

Uh oh!

Jefffrey commented Nov 2, 2025

Uh oh!

sdf-jkl commented Nov 2, 2025

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Feat: Support array flatten() on `List(LargeList(_))` types #18363

Feat: Support array flatten() on `List(LargeList(_))` types #18363

sdf-jkl commented Oct 29, 2025 •

edited

Loading