Change `flatten` so it does only a level, not recursively by velaskuez · Pull Request #15160 · apache/datafusion

velaskuez · 2025-03-11T14:45:51Z

Which issue does this PR close?

Closesflatten should be single-step, not recursive #13757

Rationale for this change

Parity with the flatten implementation in duckdb.

What changes are included in this PR?

Remove the recursion in flatten_internal so that only the top level elements are flattened.

Are these changes tested?

Existing sqllogictests have been updated.

Are there any user-facing changes?

Yes, flatten no longer recursively flattens the array

Weijun-H · 2025-03-12T05:03:27Z

+ _ => arg_types[0].clone(),
+ },
+ LargeList(field) => match field.data_type() {
+ LargeList(field) => LargeList(Arc::clone(field)),


It deserves to return LargeList, if the nested data is List
Suggested change
LargeList(field)=> LargeList(Arc::clone(field)),
List(field)| FixedSizeList(field, _) | LargeList(field) => {
LargeList(Arc::clone(field))
}

Ah ok, I'll need to make some changes to support this. Currently trying to flatten LargeList(List) will fail when casting the inner list to generic list with i64 offset (using the type parameter O)

findepi · 2025-03-12T15:50:16Z

 signature: Signature {
- // TODO (https://github.com/apache/datafusion/issues/13757) flatten should be single-step, not recursive
 type_signature: TypeSignature::ArraySignature(
 ArrayFunctionSignature::RecursiveArray,


Is this signature still applicable?
Maybe we should switch to Array, and mark RecursiveArray as deprecated?

What would the deprecated note be since, from what I understand, this was added in specifically for flatten to recursively coerce FixedLengthList to List. I'm wondering if any users would rely on that downstream

cht42 · 2025-04-02T06:10:17Z

Hey @delamarch3 are you still tracking merging this ? interested in this new behavior

velaskuez · 2025-04-02T14:06:24Z

Hi @cht42, I'm just waiting for another review.

@findepi could you take another look at this when you get the chance please?

cht42 · 2025-04-09T09:13:44Z

Maybe @alamb or @Weijun-H can review ?

Weijun-H

Thanks @delamarch3 👍 , I think the pr looks good. Waiting for other comments.

cht42

Hey @delamarch3 , noticed that null handling is not managed properly in the current implementation. Might as well fix it here directly

cht42 · 2025-04-11T14:05:15Z

- let list_arr = as_list_array(&array)?;
- let flattened_array = flatten_internal::<i32>(list_arr.clone(), None)?;
- Ok(Arc::new(flattened_array) as ArrayRef)
+ let (field, offsets, values, _) = as_list_array(&array)?.clone().into_parts();


Suggested change
let(field, offsets, values,_) = as_list_array(&array)?.clone().into_parts();
let(field, offsets, values,nulls) = as_list_array(&array)?.clone().into_parts();

cht42 · 2025-04-11T14:05:27Z

+ inner_field,
+ offsets,
+ inner_values,
+ None,


Suggested change
None,
nulls,

cht42 · 2025-04-11T14:05:41Z

- let list_arr = as_large_list_array(&array)?;
- let flattened_array = flatten_internal::<i64>(list_arr.clone(), None)?;
- Ok(Arc::new(flattened_array) as ArrayRef)
+ let (field, offsets, values, _) =


Suggested change
let(field, offsets, values,_) =
let(field, offsets, values,nulls) =

cht42 · 2025-04-11T14:05:48Z

+ inner_field,
+ offsets,
+ inner_values,
+ None,


Suggested change
None,
nulls,

cht42 · 2025-04-11T14:05:55Z

+ Ok(Arc::new(flattened_array) as ArrayRef)
+ }
+ LargeList(_) => {
+ let (inner_field, inner_offsets, inner_values, _) =


Suggested change
let(inner_field, inner_offsets, inner_values,_) =
let(inner_field, inner_offsets, inner_values,nulls) =

cht42 · 2025-04-11T14:06:05Z

+ inner_field,
+ offsets,
+ inner_values,
+ None,


Suggested change
None,
nulls,

velaskuez · 2025-04-12T09:58:48Z

Thanks for the reviews!

cht42 · 2025-04-16T07:53:01Z

@alamb can we get this merged please ?

alamb · 2025-04-17T16:53:59Z

Thanks @cht42 @delamarch3 and @Weijun-H -- I reviewed this PR and marked it as an API change. I think it looks good and does what is requested

Thanks!

* flatten array in a single step instead of recursive * clippy * update flatten type signature to Array * add fixed list to list coercion to flatten signature * support LargeList(List) and LargeList(FixedSizeList) in flatten * add test for LargeList(FixedSizeList) * handle nulls * uncomment flatten(NULL) test - it already works

flatten array in a single step instead of recursive
0417ac4

github-actionsBot added the sqllogictest SQL Logic Tests (.slt) label Mar 11, 2025

clippy
8be0a0b

alamb requested a review from findepi March 11, 2025 18:56

Weijun-H reviewed Mar 12, 2025
View reviewed changes

findepi reviewed Mar 12, 2025
View reviewed changes

velaskuez added 3 commits March 13, 2025 12:55

update flatten type signature to Array
25ee411

add fixed list to list coercion to flatten signature
2c151f4

support LargeList(List) and LargeList(FixedSizeList) in flatten
517334f

velaskuez requested review from Weijun-H and findepi March 15, 2025 15:31

add test for LargeList(FixedSizeList)
77bacd1

Weijun-H approved these changes Apr 10, 2025
View reviewed changes

cht42 reviewed Apr 11, 2025
View reviewed changes

velaskuez added 2 commits April 12, 2025 10:55

handle nulls
59bd238

uncomment flatten(NULL) test - it already works
9238722

alamb changed the title ~~Flatten array in a single step instead of recursive~~Change flatten so it does only a level, not recursivelyApr 17, 2025

alamb added the api change Changes the API exposed to users of the crate label Apr 17, 2025

alamb merged commit a0b0221 into apache:mainApr 17, 2025

joroKr21 mentioned this pull request Apr 30, 2025
Saner handling of nulls inside arrays #15149
Merged

This was referenced Apr 30, 2025
Fix flatten scalar function when inner list is FixedSizeList#15898
Merged
Main is broken #15896
Closed

joroKr21 mentioned this pull request Apr 30, 2025
Handle FixedSizeList in flatten #15899
Closed

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

	let(field, offsets, values,_) = as_list_array(&array)?.clone().into_parts();
	let(field, offsets, values,nulls) = as_list_array(&array)?.clone().into_parts();

	let(field, offsets, values,_) =
	let(field, offsets, values,nulls) =

	let(inner_field, inner_offsets, inner_values,_) =
	let(inner_field, inner_offsets, inner_values,nulls) =

Uh oh!

Conversation

velaskuez commented Mar 11, 2025

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!

Weijun-HMar 12, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cht42 commented Apr 2, 2025

Uh oh!

velaskuez commented Apr 2, 2025

Uh oh!

cht42 commented Apr 9, 2025

Uh oh!

Weijun-H left a comment

Choose a reason for hiding this comment

Uh oh!

cht42 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

velaskuez commented Apr 12, 2025

Uh oh!

cht42 commented Apr 16, 2025

Uh oh!

alamb commented Apr 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Weijun-HMar 12, 2025•
edited
Loading

cht42 left a comment •
edited
Loading