Replace `array_contains` with SQL array functions: `array_has`, `array_has_any`, `array_has_all` #6990

jayzhan211 · 2023-07-17T00:27:11Z

Which issue does this PR close?

Ref #6980

Close #6973
Close #6976
Close #6977

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jayzhan211

Not sure why I got these three files after regen.sh

proto_descriptor.bin
src/datafusion.rs serde.rs
@izveigor Did you have these three files when you run ./datafusion/proto/regen.sh before?

izveigor · 2023-07-18T11:03:56Z

@jayzhan211 Afrer all the calls the function regen.sh I have never got the files. I think it would be better to add them to .gitignore to avoid such a situation in the future.

jayzhan211 · 2023-07-20T06:29:02Z

datafusion/physical-expr/src/array_expressions.rs

-                    concat_inner_lists(array_concat(&[as_list_array(&arg)?
-                        .values()
-                        .clone()])?)
+/// Array_has_any SQL function


Did not find a way that combines has_any and has_all without losing readability.

datafusion/physical-expr/src/array_expressions.rs

jayzhan211 · 2023-07-20T08:23:10Z

@izveigor @alamb Ready for review, thanks

izveigor · 2023-07-20T14:15:33Z

@jayzhan211 Nice features! Could you add documentation (files: docs/source/user-guide/expressions.md and docs/source/user-guide/sql/scalar_functions.md) and aliases (such as array_contains and prefix list_) for the cool new functions?

datafusion/physical-expr/src/array_expressions.rs

alamb

Should we add list_has, list_has_any, list_has_all aliases to be consistent with #7008 from @izveigor ?

For anyone else following along array_contains was introduced by @izveigor in #6618 so I don't think it has been released yet (and thus this isn't an API change)

datafusion/core/tests/sqllogictests/test_files/array.slt

alamb

Thank you @jayzhan211

I think this PR looks good to me other than the panic's instead of errors. Once those are fixed I think this one is good to go.

Also, thank you to both you and @izveigor for pushing these functions ahead -- it is great to see DataFusion grow so nicely

alamb · 2023-07-20T18:55:57Z

datafusion/physical-expr/src/array_expressions.rs

+            non_list_contains!(array, args[1], BooleanArray)
+        }
+        _ => {
+            todo!(


Can we please return DataFusionError::NotYetImplemented here rather than todo!(), which will panic?

alamb · 2023-07-20T18:56:32Z

datafusion/physical-expr/src/array_expressions.rs

+                .iter()
+                .dedup()
+                .flatten()
+                .any(|x| x == elem.expect("null type not supported"));


Again, can we please try and avoid panic's on not supported errors -- it makes for a much easier to understand error message for users

datafusion/physical-expr/src/array_expressions.rs

alamb · 2023-07-20T18:59:36Z

I think we should also update the user guide https://github.com/apache/arrow-datafusion/blob/main/docs/source/user-guide/sql/scalar_functions.md#array-functions as well to include these functions

Signed-off-by: jayzhan211 <[email protected]>

docs/source/user-guide/sql/scalar_functions.md

docs/source/user-guide/expressions.md

Signed-off-by: jayzhan211 <[email protected]>

izveigor

LGTM! Thanks, @jayzhan211!

alamb

Thank you @jayzhan211 and @izveigor

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jul 17, 2023

jayzhan211 commented Jul 17, 2023

View reviewed changes

jayzhan211 mentioned this pull request Jul 17, 2023

chore: break earlier in macro contains! #6989

Merged

jayzhan211 force-pushed the array_has branch from e8c3835 to 6a01e21 Compare July 18, 2023 01:08

jayzhan211 force-pushed the array_has branch from 6a01e21 to 406fe0d Compare July 20, 2023 06:22

jayzhan211 commented Jul 20, 2023

View reviewed changes

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

jayzhan211 marked this pull request as ready for review July 20, 2023 08:21

izveigor reviewed Jul 20, 2023

View reviewed changes

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

alamb changed the title ~~Introduce SQL array function: array has~~ Introduce SQL array functions: array_has, array_has_any, array_has_all Jul 20, 2023

alamb changed the title ~~Introduce SQL array functions: array_has, array_has_any, array_has_all~~ Introduce SQL array functions: array_has, array_has_any, array_has_all, remove array_contains Jul 20, 2023

alamb changed the title ~~Introduce SQL array functions: array_has, array_has_any, array_has_all, remove array_contains~~ Replace array_contains with SQL array functions: array_has, array_has_any, array_has_all Jul 20, 2023

alamb reviewed Jul 20, 2023

View reviewed changes

datafusion/core/tests/sqllogictests/test_files/array.slt Show resolved Hide resolved

alamb reviewed Jul 20, 2023

View reviewed changes

jayzhan211 added 9 commits July 21, 2023 08:47

rename array_contains to array_has_all

bfa8a7d

Signed-off-by: jayzhan211 <[email protected]>

array_has_all with columns done

48a3a33

Signed-off-by: jayzhan211 <[email protected]>

add more tests

780ac00

Signed-off-by: jayzhan211 <[email protected]>

prepare array has any and array has

7a0d4da

Signed-off-by: jayzhan211 <[email protected]>

clippy

b9e1343

Signed-off-by: jayzhan211 <[email protected]>

add proto only

ae6dda6

Signed-off-by: jayzhan211 <[email protected]>

add generated files

6b63651

Signed-off-by: jayzhan211 <[email protected]>

basic array_has done

b91ee91

Signed-off-by: jayzhan211 <[email protected]>

add more tests

24dc3cb

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 added 6 commits July 21, 2023 08:47

basic array_has done

e93db47

Signed-off-by: jayzhan211 <[email protected]>

array_has_all done

40a0894

Signed-off-by: jayzhan211 <[email protected]>

any and all are done

2900291

Signed-off-by: jayzhan211 <[email protected]>

remove proto files

e6fccdc

Signed-off-by: jayzhan211 <[email protected]>

remove test

e76ced9

Signed-off-by: jayzhan211 <[email protected]>

address comment

01fab56

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 force-pushed the array_has branch from 10eca26 to 01fab56 Compare July 21, 2023 01:51

run doc command

855064a

Signed-off-by: jayzhan211 <[email protected]>

Weijun-H reviewed Jul 21, 2023

View reviewed changes

docs/source/user-guide/sql/scalar_functions.md Outdated Show resolved Hide resolved

Weijun-H reviewed Jul 21, 2023

View reviewed changes

docs/source/user-guide/expressions.md Outdated Show resolved Hide resolved

jayzhan211 requested review from izveigor and alamb July 21, 2023 04:03

rerun command

4e5361d

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 force-pushed the array_has branch from f69b56b to 4e5361d Compare July 21, 2023 07:56

izveigor approved these changes Jul 21, 2023

View reviewed changes

alamb approved these changes Jul 22, 2023

View reviewed changes

alamb merged commit 49c91b5 into apache:main Jul 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `array_contains` with SQL array functions: `array_has`, `array_has_any`, `array_has_all` #6990

Replace `array_contains` with SQL array functions: `array_has`, `array_has_any`, `array_has_all` #6990

jayzhan211 commented Jul 17, 2023 •

edited

Loading

jayzhan211 left a comment •

edited

Loading

izveigor commented Jul 18, 2023 •

edited

Loading

jayzhan211 Jul 20, 2023

jayzhan211 commented Jul 20, 2023

izveigor commented Jul 20, 2023

alamb left a comment

alamb left a comment

alamb Jul 20, 2023

alamb Jul 20, 2023

alamb commented Jul 20, 2023

izveigor left a comment

alamb left a comment

Replace array_contains with SQL array functions: array_has, array_has_any, array_has_all #6990

Replace array_contains with SQL array functions: array_has, array_has_any, array_has_all #6990

Conversation

jayzhan211 commented Jul 17, 2023 • edited Loading

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?

jayzhan211 left a comment • edited Loading

Choose a reason for hiding this comment

izveigor commented Jul 18, 2023 • edited Loading

jayzhan211 Jul 20, 2023

Choose a reason for hiding this comment

jayzhan211 commented Jul 20, 2023

izveigor commented Jul 20, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jul 20, 2023

Choose a reason for hiding this comment

alamb Jul 20, 2023

Choose a reason for hiding this comment

alamb commented Jul 20, 2023

izveigor left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Replace `array_contains` with SQL array functions: `array_has`, `array_has_any`, `array_has_all` #6990

Replace `array_contains` with SQL array functions: `array_has`, `array_has_any`, `array_has_all` #6990

jayzhan211 commented Jul 17, 2023 •

edited

Loading

jayzhan211 left a comment •

edited

Loading

izveigor commented Jul 18, 2023 •

edited

Loading