Skip to content

Add ARRAYS_OVERLAP_COUNT implementation, documentation and tests #24926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add ARRAYS_OVERLAP_COUNT implementation, documentation and tests #24926

wants to merge 1 commit into from

Conversation

Leziak
Copy link
Contributor

@Leziak Leziak commented Apr 16, 2025 •
edited
Loading

Description

Requested in #24911

I went with the simplest and least error-prone approach and simply make the function invoke the intersect function and return the amount of items in the returned array.

Motivation and Context

See #24911

Impact

See #24911

Test Plan

image

Wrote a lot of tests, the vast majority of which I adapted from TestArrayIntersectFunction

Should I also benchmark it? Don't know how exactly that's done here, would appreciate some pointers, thanks!

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes

  • Add :func:arrays_overlap_count(x, y) -> bigint() function to return the amount of overlapping elements between array x and y without duplicates.

@Leziak Leziak requested review from steveburnett, elharo and a team as code owners April 16, 2025 20:38
@Leziak Leziak requested a review from presto-oss April 16, 2025 20:38
kaikalur
kaikalur requested changes Apr 16, 2025
View reviewed changes
...-main-base/src/main/java/com/facebook/presto/operator/scalar/ArraysOverlapCountFunction.java Outdated
@SqlType("array(E)") Block leftArray,
@SqlType("array(E)") Block rightArray)
{
return ArrayIntersectFunction.intersect(type, elementIsDistinctFrom, leftArray, rightArray).getPositionCount();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will be good to avoid calling intersect as it will create a new block

steveburnett
steveburnett previously approved these changes Apr 16, 2025
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (docs)

Pull branch, local doc build, looks good. Thanks!

@steveburnett
Copy link
Contributor

You could include a release note for this, and link to the documentation for the new function as shown in the following example:

== RELEASE NOTES ==

General Changes
* Add :func:`arrays_overlap_count(x, y) -> bigint()` function to return the amount of overlapping elements between array ``x`` and ``y`` without duplicates.

@Leziak
Copy link
Contributor Author

Leziak commented Apr 16, 2025

@steveburnett looks like another intermittent test failure, could you re-run this one as well please? thanks!

@Leziak Leziak force-pushed the array-overlap-count branch from 44f79f3 to c9b1439 Compare April 18, 2025 14:32
Leziak
Leziak commented Apr 18, 2025
View reviewed changes
...n-base/src/test/java/com/facebook/presto/operator/scalar/TestArraysOverlapCountFunction.java
}

@Test
public void testIndeterminateRows()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all other tests besides this one pass, would it be okay if we landed this function with the caveat that indeterminate rows aren't supported? I'm pretty sure they'll be a very small subset of inputs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uploading image.png…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have been trying to standardize on IS DISTINCT FROM like semantics for NULLs. @rschlussel - can you confirm?

@Leziak Leziak force-pushed the array-overlap-count branch from c9b1439 to 9711d4c Compare April 18, 2025 14:36
@kaikalur kaikalur requested a review from rschlussel April 18, 2025 18:28
@Leziak Leziak force-pushed the array-overlap-count branch 2 times, most recently from 50e23aa to 2a5830c Compare April 18, 2025 20:08
kaikalur
kaikalur previously approved these changes Apr 18, 2025
View reviewed changes
@Leziak Leziak force-pushed the array-overlap-count branch from 2a5830c to 93e0c37 Compare April 18, 2025 22:23
kaikalur
kaikalur reviewed Apr 18, 2025
View reviewed changes
Copy link
Contributor

@kaikalur kaikalur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you revert the previous change? was there an issue?

@Leziak
Copy link
Contributor Author

Leziak commented Apr 19, 2025

Did you revert the previous change? was there an issue?

yeah reverted it - it made the duplicates test fail, reverting it fixed it (besides the test rows and arrays with null with them, which are still failing)

@kaikalur
Copy link
Contributor

Did you revert the previous change? was there an issue?

yeah reverted it - it made the duplicates test fail, reverting it fixed it (besides the test rows and arrays with null with them, which are still failing)

Hmm - that's odd! That logic is supposed to be sound!

@steveburnett
Copy link
Contributor

What needs to be addressed in the PR to move it towards merging? Thanks!

@Leziak
Copy link
Contributor Author

Leziak commented Apr 28, 2025

What needs to be addressed in the PR to move it towards merging? Thanks!

I think we're waiting for @rschlussel to confirm whether I can go ahead with the current implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaikalur kaikalur kaikalur left review comments

@steveburnett steveburnett steveburnett left review comments

@elharo elharo Awaiting requested review from elharo elharo is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@rschlussel rschlussel Awaiting requested review from rschlussel

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Leziak @steveburnett @kaikalur