Support array_distinct function. #8268

my-vegetable-has-exploded · 2023-11-19T13:14:46Z

Which issue does this PR close?

Closes #7289

Rationale for this change

just use list.iter().sorted().dedup() to remove duplicates for each list in listarray

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

2010YOUY01

Thanks for the new function support, I did some tests and have a few suggestions:

❯ select array_distinct([]);
Optimizer rule 'simplify_expressions' failed
caused by
Internal error: could not cast value to arrow_array::array::list_array::GenericListArray<i32>.
This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

I think this empty array case should be handled inside implementation (and also included in sqllogictest)

datafusion/expr/src/built_in_function.rs

alamb · 2023-11-21T20:33:16Z

I merged #8269 so we can probably pick up the change for this PR

my-vegetable-has-exploded · 2023-11-23T02:23:24Z

PTAL, @alamb @jayzhan211 @2010YOUY01 , thanks.

Weijun-H

Thanks 👍

datafusion/physical-expr/src/array_expressions.rs

datafusion/sqllogictest/test_files/array.slt

datafusion/physical-expr/src/array_expressions.rs

zhangxffff · 2023-11-29T09:36:48Z

datafusion/physical-expr/src/array_expressions.rs

+    let converter = RowConverter::new(vec![SortField::new(dt.clone())])?;
+    // distinct for each list in ListArray
+    for arr in array.iter().flatten() {
+        let values = converter.convert_columns(&[arr])?;


why not distinct array in columnar way, arr has only one column, using row format need extra encoding and decoding

why not distinct array in columnar way, arr has only one column, using row format need extra encoding and decoding

It is great to distinct array without row converter, but I don't think we can do that without downcast to exact arr then do the distinction. Is there any recommended way?

I also don't have another idea other than downcast arr, I was just wondering if it is worth to downcast to exact arr.

I also don't have another idea other than downcast arr, I was just wondering if it is worth to downcast to exact arr.

Downcasting to the exact array type can result in faster code in many cases, as the rust compiler can make specialized implemenations for each type. However, there are a lot of DataTypes, including nested ones like Dict, List, Struct, etc so making specialized implementations often requires a lot of work

The row converter handles all the types internally.

What we have typically done in the past with DataFusion is to use non type specific code like RowConverter for the general case, and then if we find a particular usecase needs faster performance we make special implementations. For example, we do so for grouing by single primtive columns (GROUP BY int32) for example

jayzhan211

LGTM

datafusion/physical-expr/src/array_expressions.rs

implement slt & proto fix null & empty list

Co-authored-by: Alex Huang <huangweijun1001@gmail.com>

my-vegetable-has-exploded · 2023-12-05T12:17:28Z

Since no more comments for a fews days, I think maybe this pr can go ahead?
cc @alamb, thanks.

alamb · 2023-12-06T21:50:15Z

Thanks @my-vegetable-has-exploded -- I'll take a look hopefully today or maybe tomorrow

Weijun-H · 2023-12-07T11:47:22Z

datafusion/physical-expr/src/array_expressions.rs

+            let array = as_list_array(&args[0])?;
+            general_array_distinct(array, field)


nit: put let array = as_list_array(&args[0])?; in general_array_distinct

Sorry, I don't get your point. Iargelist differs with list, so I think it maybe better to handle it before generic function?

I mean change the function signature:

general_array_distinct<OffsetSize: OffsetSizeTrait>( array: &ArrayRef, field: &FieldRef, )

Then cast array in the fucntion

I think you are referring to something like general_array_has_dispatch

I think we can merge this PR as is and then add support for LargeList (using the OffsetSize trait) as a follow on PR

alamb

THank you for this contribution @my-vegetable-has-exploded and thank you @Weijun-H and @jayzhan211 for the help getting this PR ready.

I think it looks very nice and is a good example of collaboration 🦾

alamb · 2023-12-08T17:39:26Z

datafusion/physical-expr/src/array_expressions.rs

+    let converter = RowConverter::new(vec![SortField::new(dt.clone())])?;
+    // distinct for each list in ListArray
+    for arr in array.iter().flatten() {
+        let values = converter.convert_columns(&[arr])?;


I also don't have another idea other than downcast arr, I was just wondering if it is worth to downcast to exact arr.

Downcasting to the exact array type can result in faster code in many cases, as the rust compiler can make specialized implemenations for each type. However, there are a lot of DataTypes, including nested ones like Dict, List, Struct, etc so making specialized implementations often requires a lot of work

The row converter handles all the types internally.

What we have typically done in the past with DataFusion is to use non type specific code like RowConverter for the general case, and then if we find a particular usecase needs faster performance we make special implementations. For example, we do so for grouing by single primtive columns (GROUP BY int32) for example

alamb · 2023-12-08T17:40:21Z

datafusion/physical-expr/src/array_expressions.rs

+            let array = as_list_array(&args[0])?;
+            general_array_distinct(array, field)


I think we can merge this PR as is and then add support for LargeList (using the OffsetSize trait) as a follow on PR

alamb · 2023-12-08T17:42:56Z

I took the liberty of merging up from main to make sure there there are no logical conflicts. I intend to merge the PR when the tests pass

my-vegetable-has-exploded · 2023-12-09T11:55:14Z

Thanks all.

* implement distinct func implement slt & proto fix null & empty list * add comment for slt Co-authored-by: Alex Huang <huangweijun1001@gmail.com> * fix largelist * add largelist for slt * Use collect for rows & init capcity for offsets. * fixup: remove useless match * fix fmt * fix fmt --------- Co-authored-by: Alex Huang <huangweijun1001@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Nov 19, 2023

my-vegetable-has-exploded changed the title ~~Minor: Implement array_distinct function.~~ Minor: Support array_distinct function. Nov 19, 2023

2010YOUY01 reviewed Nov 20, 2023

View reviewed changes

datafusion/expr/src/built_in_function.rs Show resolved Hide resolved

alamb mentioned this pull request Nov 20, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 20, 2023 #8274

Closed

7 tasks

my-vegetable-has-exploded force-pushed the array-distinct branch from fe5bd05 to 1e615b2 Compare November 22, 2023 04:58

my-vegetable-has-exploded requested review from 2010YOUY01 and jayzhan211 November 22, 2023 05:12

my-vegetable-has-exploded force-pushed the array-distinct branch from 1e615b2 to ec3d443 Compare November 27, 2023 03:11

Weijun-H reviewed Nov 27, 2023

View reviewed changes

datafusion/physical-expr/src/array_expressions.rs Outdated Show resolved Hide resolved

datafusion/sqllogictest/test_files/array.slt Show resolved Hide resolved

alamb mentioned this pull request Nov 27, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 27, 2023 #8329

Closed

8 tasks

my-vegetable-has-exploded force-pushed the array-distinct branch from 5f42d9c to 86ccb87 Compare November 27, 2023 12:42

my-vegetable-has-exploded requested a review from Weijun-H November 27, 2023 13:24

zhangxffff reviewed Nov 29, 2023

View reviewed changes

my-vegetable-has-exploded force-pushed the array-distinct branch from ccfed93 to 9e22960 Compare November 29, 2023 13:57

my-vegetable-has-exploded requested a review from zhangxffff November 29, 2023 13:58

jayzhan211 approved these changes Nov 30, 2023

View reviewed changes

datafusion/physical-expr/src/array_expressions.rs Outdated Show resolved Hide resolved

my-vegetable-has-exploded force-pushed the array-distinct branch from 9e22960 to 62f11f5 Compare November 30, 2023 02:47

my-vegetable-has-exploded changed the title ~~Minor: Support array_distinct function.~~ Support array_distinct function. Nov 30, 2023

my-vegetable-has-exploded requested a review from jayzhan211 November 30, 2023 03:17

jayzhan211 approved these changes Nov 30, 2023

View reviewed changes

my-vegetable-has-exploded and others added 6 commits December 5, 2023 19:22

implement distinct func

430caa2

implement slt & proto fix null & empty list

add comment for slt

2ffc9f8

Co-authored-by: Alex Huang <huangweijun1001@gmail.com>

fix largelist

c777184

add largelist for slt

009d6de

Use collect for rows & init capcity for offsets.

7f0152e

fixup: remove useless match

c2f5451

my-vegetable-has-exploded force-pushed the array-distinct branch from 62f11f5 to c2f5451 Compare December 5, 2023 11:27

fix fmt

ac33215

Weijun-H approved these changes Dec 5, 2023

View reviewed changes

alamb mentioned this pull request Dec 6, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 4, 2023 #8420

Closed

7 tasks

my-vegetable-has-exploded added 2 commits December 7, 2023 12:36

Merge remote-tracking branch 'origin/main' into array-distinct

18a6e84

fix fmt

9d8bc00

Weijun-H reviewed Dec 7, 2023

View reviewed changes

alamb approved these changes Dec 8, 2023

View reviewed changes

Merge remote-tracking branch 'apache/main' into array-distinct

c5960dd

alamb merged commit cd02c40 into apache:main Dec 8, 2023

my-vegetable-has-exploded deleted the array-distinct branch December 9, 2023 11:55

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

		let array = as_list_array(&args[0])?;
		general_array_distinct(array, field)

Support array_distinct function. #8268

Support array_distinct function. #8268

Uh oh!

Conversation

my-vegetable-has-exploded commented Nov 19, 2023

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!

2010YOUY01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Nov 21, 2023

Uh oh!

my-vegetable-has-exploded commented Nov 23, 2023

Uh oh!

Weijun-H left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Nov 29, 2023 • 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!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

my-vegetable-has-exploded commented Dec 5, 2023

Uh oh!

alamb commented Dec 6, 2023

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!

alamb commented Dec 8, 2023

Uh oh!

my-vegetable-has-exploded commented Dec 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jayzhan211 Nov 29, 2023 •

edited

Loading