Skip to content

feat: array_contains - #6618

Merged
alamb merged 7 commits into
apache:mainfrom
izveigor:array_contains
Jun 27, 2023
Merged

feat: array_contains#6618
alamb merged 7 commits into
apache:mainfrom
izveigor:array_contains

Conversation

@izveigor

@izveigorizveigor commented Jun 9, 2023

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#6557

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

@github-actionsgithub-actionsBot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Jun 9, 2023
@izveigor
izveigor marked this pull request as draft June 9, 2023 18:37
@izveigor
izveigor marked this pull request as ready for review June 23, 2023 18:59
izveigor
izveigor commented Jun 23, 2023
View reviewed changes
Comment threaddatafusion/physical-expr/src/array_expressions.rs
]))))
}

macro_rules! contains {

@izveigorizveigorJun 23, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

What the function: https://docs.rs/arrow/latest/arrow/compute/kernels/comparison/fn.in_list.html?

@alambalambJun 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't quite understand this comment -- are you saying that it would be better to use the in_list kernel rather than flattening it?

@izveigor

izveigor commented Jun 23, 2023

Copy link
Copy Markdown
ContributorAuthor

@alamb I wonder if you review this PR if you have free time.

@alamb

alamb commented Jun 24, 2023

Copy link
Copy Markdown
Contributor

I will find time to review this PR, but maybe not until Monday

alamb
alamb approved these changes Jun 26, 2023
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great @izveigor -- thank you very much for this contribution.

I agree with your comments in #6557 that it would be interesting to add operators like @> such as postgres has https://www.postgresql.org/docs/current/functions-array.html. Should we file some follow on tickets to try and crowdsource that work?

Comment threaddatafusion/physical-expr/src/array_expressions.rs
]))))
}

macro_rules! contains {

@alambalambJun 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't quite understand this comment -- are you saying that it would be better to use the in_list kernel rather than flattening it?

Comment threaddocs/source/user-guide/expressions.md
@@ -179,23 +179,24 @@ Unlike to some databases the math functions in Datafusion works the same way as

## Array Expressions

@alambalambJun 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❤️

Comment threaddocs/source/user-guide/sql/scalar_functions.md Outdated
@alamb

alamb commented Jun 26, 2023

Copy link
Copy Markdown
Contributor

The other thing I think would help this PR is some tests for the array version in .slt (rather than just scalar) but I will write those as a follow on PR (as I want to get some experience with these functions myself).

@izveigor

izveigor commented Jun 26, 2023

Copy link
Copy Markdown
ContributorAuthor

@alamb I think it would be better to create a separate array function ticket with columns (Since I considered only scalars as arguments). (See the tickets: #6693 and #6709).

@alambalamb mentioned this pull request Jun 26, 2023
Merged
@alamb

alamb commented Jun 26, 2023

Copy link
Copy Markdown
Contributor

@alamb I think it would be better to create a separate array function ticket with columns (Since I considered only scalars as arguments). (See the tickets: #6693 and #6709).

Indeed @izveigor -- in fact I found exactly the same bug when writing the tests -- #6771

@alamb
alamb merged commit 1dd1fbd into apache:mainJun 27, 2023
@alamb

alamb commented Jun 27, 2023

Copy link
Copy Markdown
Contributor

Thanks again @izveigor

maxburke pushed a commit to urbanlogiq/arrow-datafusion that referenced this pull request Jul 14, 2023
@izveigor@alamb@maxburke
feat:array_contains(apache#6618)
7fe0110
* feat: array_contains
* feat: regen.sh
* docs: array_contains
* fix: merge
* Update docs/source/user-guide/sql/scalar_functions.md
---------
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@alambalamb mentioned this pull request Jul 20, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes

Assignees

No one assigned

Labels

coreCore DataFusion cratelogical-exprLogical plan and expressionsphysical-exprChanges to the physical-expr cratessqllogictestSQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New array method array_contains

2 participants

@izveigor@alamb