feat(datafusion-functions-aggregate): add support for lists and other nested types in `min` and `max` #13991

rluvaton · 2025-01-02T20:12:19Z

Which issue does this PR close?

Rationale for this change

You are now able to run min and max on lists

What changes are included in this PR?

Added GenericMinMaxAccumulator and GenericSlidingMinMaxAccumulator which calculate the min and max based on the arrow-row crate

Are these changes tested?

For the min and max yes, for the moving min max - no (fuzzy testing will help here a lot to cover the large range of tests)

Are there any user-facing changes?

Yes, users are now allowed to sort lists and every type that is supported by the arrow-row

alamb

Thank you @rluvaton -- I reviewed this code and it looks quite nice. Thank you 👌 🏆

The only thing I think is missing is "end to end" query coverage. In addition to the empty input test, I think we need tests specifically for the sliding window min/max

Can you please add such tests as .slt tests -- it would be queries something like

SELECT min(list_col) OVER (ORDER BY other_col 1 ROW PRECEDING 1 ROW FOLLOWING) FROM t

where list_col had nulls in it

Bonus points for tests of:

struct Min/Max

Let mek nw

alamb · 2025-01-04T14:55:33Z

datafusion/functions-aggregate/src/min_max.rs

+                acc_args.return_type,
+            )?))
+        } else {
+            Ok(Box::new(MaxAccumulator::try_new(acc_args.return_type)?))


The pattern used in other accumulators is slightly different I think -- It would have a function on MaxAccumulator that explicitly declared the types it supported and then fallback to the GenericMaxAccumulator if not.

For example, something like

if MaxAccumulator::supports_type(&acc_args.return_type) { Ok(Box::new(MaxAccumulator::try_new(acc_args.return_type)?)) } else { Ok(Box::new(GenericMaxAccumulator::try_new( acc_args.return_type, )?)) }

This is a minor detail and not required, I just wanted to point it out

alamb · 2025-01-04T14:56:12Z

datafusion/functions-aggregate/src/min_max.rs

+            Ok(Box::new(GenericMaxAccumulator::try_new(
+                acc_args.return_type,
+            )?))


If you wanted to make this less verbose you could probably do something like

Suggested change

Ok(Box::new(GenericMaxAccumulator::try_new(

acc_args.return_type,

)?))

GenericMaxAccumulator::try_new(

acc_args.return_type,

).map(Box::new)

alamb · 2025-01-04T15:00:46Z

datafusion/functions-aggregate/src/min_max/min_max_generic.rs

+    }
+
+    fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
+        // TODO - should assert getting only one column?


I think it would be nice to either add the assert or remove the TODO as part of this PR

alamb · 2025-01-04T15:04:14Z

datafusion/sqllogictest/test_files/array_query.slt

@@ -108,11 +108,15 @@ SELECT * FROM data WHERE column2 is not distinct from null;
 # Aggregates
 ###########

-query error Internal error: Min/Max accumulator not implemented for type List
+query ?
 SELECT min(column1) FROM data;


Can we also add tests for empty input on min/max as well to test boundary conditions? Something like

SELECT MIN(column1) FROM data WHERE false

Which should return a null value?

jayzhan211 · 2025-01-10T01:29:44Z

datafusion/functions-aggregate/src/min_max/min_max_generic.rs

+
+        // Create a null row to use for filtering out nulls from the input
+        let null_row = {
+            let null_array = ScalarValue::try_from(datatype)?.to_array_of_size(1)?;


Suggested change

let null_array = ScalarValue::try_from(datatype)?.to_array_of_size(1)?;

let null_array = new_null_array(datatype, 1);

jayzhan211 · 2025-01-10T01:39:05Z

datafusion/functions-aggregate/src/min_max/min_max_generic.rs

+    }
+
+    #[test]
+    fn basic_i32_list() {


statement count 0 create table t1 (a int[]) as values ([1,2,3]), ([1,2,1]); query ? select min(a) from t1; ---- [1, 2, 1] statement count 0 drop table t1;

You can create test like this, it is easier to maintain than this rust test

alamb · 2025-01-16T22:22:41Z

Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look

Specifically I think this PR just needs some more tests and it will be good to go

github-actions · 2025-03-18T02:03:35Z

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

ologlogn · 2025-04-09T12:48:30Z

Hey @alamb @jayzhan211, I see that this PR is closed, is it possible to reopen this and complete this feature?
Are there any concerns apart from the tests? We can also take up this code and finish missing requirements.
Issue: #15477
cc @gabotechs @LiaCastaneda

alamb · 2025-04-10T18:45:29Z

Hey @alamb @jayzhan211, I see that this PR is closed, is it possible to reopen this and complete this feature? Are there any concerns apart from the tests? We can also take up this code and finish missing requirements. Issue: #15477 cc @gabotechs @LiaCastaneda

Thanks @ologlogn ! From my perspective this test simply needed some more slt tests

Specifically I think this PR just needs some more tests and it will be good to go

What I would suggest is creating a new PR using the code in this branch as a starting point and then add more tests, get it ready for review.

fyi @rluvaton

rluvaton · 2025-04-10T18:46:26Z

I did not meant to close it, sorry, it is in the back of my mind, I'll finish this now, @ologlogn don't take it 😀

ologlogn · 2025-04-11T13:37:45Z

Thanks @rluvaton! It will be really helpful :)

gabotechs · 2025-04-24T06:35:07Z

@rluvaton do you have an estimate of when this might be shipped? We’re currently blocked by this, so we’d be glad to handle it ourselves if that would help ease your workload.

rluvaton · 2025-04-24T06:36:21Z

I'm really sorry, had crazy week with the baby, will work on it today

rluvaton · 2025-04-24T17:05:36Z

Sorry, I could not find time today, you can take this but please add me as co-author for my changes:

Creating a commit with multiple authors

gabotechs · 2025-04-25T10:11:03Z

Work resumed in #15857

rluvaton added 5 commits January 2, 2025 19:59

add support for lists in min

dba5552

add support for lists in max

ed910e4

reuse code

2a8ea48

add comment

c92a1be

format and clippy

9b2370b

github-actions bot added the functions Changes to functions implementation label Jan 2, 2025

rluvaton changed the title ~~add support for lists in min~~ feat(datafusion-functions-aggregate): add support for lists and other nested types in min and max Jan 2, 2025

rluvaton added 2 commits January 2, 2025 20:20

added tests and license in header

c997b21

update array_query

6990c4e

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

rluvaton marked this pull request as ready for review January 2, 2025 20:35

alamb reviewed Jan 4, 2025

View reviewed changes

jayzhan211 reviewed Jan 10, 2025

View reviewed changes

alamb marked this pull request as draft January 16, 2025 22:22

github-actions bot added the Stale PR has not had any activity for some time label Mar 18, 2025

github-actions bot closed this Mar 25, 2025

gabotechs mentioned this pull request Apr 25, 2025

feat(datafusion-functions-aggregate): add support for lists and other nested types in min and max #15857

Closed

	let null_array = ScalarValue::try_from(datatype)?.to_array_of_size(1)?;
	let null_array = new_null_array(datatype, 1);

feat(datafusion-functions-aggregate): add support for lists and other nested types in min and max #13991

feat(datafusion-functions-aggregate): add support for lists and other nested types in min and max #13991

Uh oh!

Conversation

rluvaton commented Jan 2, 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!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented Jan 16, 2025

Uh oh!

github-actions bot commented Mar 18, 2025

Uh oh!

ologlogn commented Apr 9, 2025

Uh oh!

alamb commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rluvaton commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ologlogn commented Apr 11, 2025

Uh oh!

gabotechs commented Apr 24, 2025

Uh oh!

rluvaton commented Apr 24, 2025

Uh oh!

rluvaton commented Apr 24, 2025

Uh oh!

gabotechs commented Apr 25, 2025

Uh oh!

Uh oh!

feat(datafusion-functions-aggregate): add support for lists and other nested types in `min` and `max` #13991

feat(datafusion-functions-aggregate): add support for lists and other nested types in `min` and `max` #13991

rluvaton commented Jan 2, 2025 •

edited

Loading

alamb commented Apr 10, 2025 •

edited

Loading

rluvaton commented Apr 10, 2025 •

edited

Loading