Add array_contains_all function by jainavi17 · Pull Request #23202 · prestodb/presto

jainavi17 · 2024-07-13T07:01:30Z

Core function to return whether all elements of a second array are present in the first array

Test plan

Added unit tests.
Build successfully using the following terminal command

mvn clean install -Dtest=TestArrayOperators -Dmaven.javadoc.skip=true -DskipUI -T1C -fn -pl presto-main

== RELEASE NOTES ==

General Changes
- Add function :func:`array_contains_array`

elharo

Implementation looks good. I think this needs an issue, and perhaps an RFC.

Also if it's in the code is it a UDF or just a function?

presto-docs/src/main/sphinx/functions/array.rst

presto-main/src/test/java/com/facebook/presto/type/TestArrayOperators.java

kaikalur · 2024-07-13T14:41:19Z

I suggest array_contains_all as the name

presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayContainsAllFunction.java

steveburnett

Thanks for the doc! Nit only.

presto-docs/src/main/sphinx/functions/array.rst

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, docs look good. Thanks!

presto-docs/src/main/sphinx/functions/array.rst

presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayContainsAllFunction.java

presto-docs/src/main/sphinx/functions/array.rst

rschlussel · 2024-08-22T14:27:41Z

presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayContainsAllFunction.java

+            @SqlType("array(T)") Block firstArray,
+            @SqlType("array(T)") Block secondArray)
+    {
+        TypedSet firstSet = new TypedSet(elementType, firstArray.getPositionCount(), "arrayContainsAll");


Can you benchmark this version vs. using OptimizedTypeSet and checking the cardinality of the intersection of the two sets? I expect that using OptimizedTypedSet would be faster except for the case where a large second array is able to short circuit very early.

Actually curious - how about CONTAINS? @rschlussel did you fix that as well to be IS DISTINCT FROM? This function should behave like contains for that part

contains fails for all null values. If we want this to fail for all nulls, we can do that as well, but we shouldn't have failures only for nulls nested in complex types, but otherwise compare nulls as equal (which is what typedset does by default)

@jainavi17 did you have a chance to benchmark vs. a version using OptimizedTypeSet?

presto-main/src/test/java/com/facebook/presto/type/TestArrayContainsAll.java

elharo · 2024-08-22T21:16:39Z

FWIW, if anything, this function does not exist in Spark SQL, so adding this function increases the divergence between Presto SQL and Spark SQL. I don't know how much weight that carries here, though I do know some people care about this

kaikalur · 2024-08-22T21:22:38Z

FWIW, if anything, this function does not exist in Spark SQL, so adding this function increases the divergence between Presto SQL and Spark SQL. I don't know how much weight that carries here, though I do know some people care about this

That's not a concern especially because adding udfs to Spark is easier

aditi-pandit · 2024-08-23T08:59:03Z

@jainavi17 : Please can you create an issue to add this function for the Native engine.

jainavi17 · 2024-09-04T16:23:03Z

@jainavi17 : Please can you create an issue to add this function for the Native engine.

Native engine as in?

steveburnett

Just a nit of formatting.

presto-docs/src/main/sphinx/functions/array.rst

Scalar function that takes two arrays an input and checks it all elements of right array are present in left array

jainavi17 · 2024-09-04T17:21:04Z

Hi @NikhilCollooru , if you could review the PR please.

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build. Thanks!

jainavi17 · 2024-09-04T20:31:29Z

@elharo if you could take a look at the changes please.

rschlussel · 2024-09-05T20:59:39Z

presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayContainsAllFunction.java

+            @SqlType("array(T)") Block firstArray,
+            @SqlType("array(T)") Block secondArray)
+    {
+        TypedSet firstSet = new TypedSet(elementType, firstArray.getPositionCount(), "arrayContainsAll");


can you use distinct semantics here so that we don't throw an exception for arrays of arrays with nulls? You pass in the "distinct" function handle to the typedSet. You can follow how it's done for array_distinct here: You can use "distinct comparison" for the typedset following what we do for array_distinct see

presto/presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayDistinctFunction.java

Line 72 in 6f28f37

TypedSet typedSet = new TypedSet(type, elementIsDistinctFrom, array.getPositionCount(), "array_distinct");

rschlussel · 2024-09-05T21:00:06Z

presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayContainsAllFunction.java

+            @SqlType("array(T)") Block firstArray,
+            @SqlType("array(T)") Block secondArray)
+    {
+        TypedSet firstSet = new TypedSet(elementType, firstArray.getPositionCount(), "arrayContainsAll");


@jainavi17 did you have a chance to benchmark vs. a version using OptimizedTypeSet?

rschlussel · 2024-09-05T21:01:24Z

presto-main/src/test/java/com/facebook/presto/type/TestArrayContainsAll.java

+    }
+
+    @Test
+    public void testNulls()


add a test for array of arrays/rows containing nulls

rschlussel · 2024-09-05T21:02:23Z

@jainavi17 : Please can you create an issue to add this function for the Native engine.

Native engine as in?

native engine means that we need a corresponding c++ function for this as we are in the process of migrating from java workers to c++ workers.

jainavi17 · 2024-10-03T08:31:39Z

Hi @rschlussel I tried to do benchmark test but somehow my IDE is breaking and I'm unable to set up the benchmark test. Is there any way I can do it on command line? Thanks

jainavi17 requested review from a team, elharo and steveburnett as code owners July 13, 2024 07:01

jainavi17 requested a review from presto-oss July 13, 2024 07:01

elharo requested changes Jul 13, 2024

View reviewed changes

presto-docs/src/main/sphinx/functions/array.rst Outdated Show resolved Hide resolved

presto-main/src/test/java/com/facebook/presto/type/TestArrayOperators.java Outdated Show resolved Hide resolved

kaikalur requested changes Jul 13, 2024

View reviewed changes

presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayContainsAllFunction.java Show resolved Hide resolved

steveburnett requested changes Jul 15, 2024

View reviewed changes

presto-docs/src/main/sphinx/functions/array.rst Outdated Show resolved Hide resolved

jainavi17 force-pushed the array_contains_array branch from 50d6889 to 62668f4 Compare August 15, 2024 09:33

steveburnett previously approved these changes Aug 15, 2024

View reviewed changes

elharo changed the title ~~Add array_contains_array function~~ Add array_contains_all function Aug 15, 2024

jainavi17 dismissed steveburnett’s stale review via 468b26a August 22, 2024 08:06

jainavi17 force-pushed the array_contains_array branch 2 times, most recently from 468b26a to f86b0f8 Compare August 22, 2024 08:08

elharo previously approved these changes Aug 22, 2024

View reviewed changes

jainavi17 requested review from elharo, kaikalur and steveburnett August 22, 2024 13:50

kaikalur requested changes Aug 22, 2024

View reviewed changes

presto-docs/src/main/sphinx/functions/array.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/functions/array.rst Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayContainsAllFunction.java Show resolved Hide resolved

rschlussel reviewed Aug 22, 2024

View reviewed changes

jainavi17 dismissed elharo’s stale review via d209068 September 4, 2024 16:50

jainavi17 force-pushed the array_contains_array branch 2 times, most recently from d209068 to a411156 Compare September 4, 2024 17:02

steveburnett requested changes Sep 4, 2024

View reviewed changes

presto-docs/src/main/sphinx/functions/array.rst Show resolved Hide resolved

Add array_contains_array function

8311dd3

Scalar function that takes two arrays an input and checks it all elements of right array are present in left array

jainavi17 force-pushed the array_contains_array branch from a411156 to 8311dd3 Compare September 4, 2024 17:17

steveburnett approved these changes Sep 4, 2024

View reviewed changes

rschlussel reviewed Sep 5, 2024

View reviewed changes

Conversation

jainavi17 commented Jul 13, 2024 • edited by elharo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaikalur commented Jul 13, 2024

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rschlussel Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

kaikalur Aug 22, 2024 • 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!

rschlussel Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

rschlussel Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elharo commented Aug 22, 2024

Uh oh!

kaikalur commented Aug 22, 2024

Uh oh!

aditi-pandit commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jainavi17 commented Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jainavi17 commented Sep 4, 2024

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

jainavi17 commented Sep 4, 2024

Uh oh!

rschlussel Sep 5, 2024 • 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!

rschlussel Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

rschlussel Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

rschlussel commented Sep 5, 2024

Uh oh!

jainavi17 commented Oct 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jainavi17 commented Jul 13, 2024 •

edited by elharo

Loading

kaikalur Aug 22, 2024 •

edited

Loading

aditi-pandit commented Aug 23, 2024 •

edited

Loading

jainavi17 commented Sep 4, 2024 •

edited

Loading

rschlussel Sep 5, 2024 •

edited

Loading