Skip to content

chore: break earlier in macro contains! - #6989

Merged
alamb merged 1 commit into
apache:mainfrom
Weijun-H:chore
Jul 17, 2023
Merged

chore: break earlier in macro contains!#6989
alamb merged 1 commit into
apache:mainfrom
Weijun-H:chore

Conversation

@Weijun-H

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actionsgithub-actionsBot added the physical-expr Changes to the physical-expr crates label Jul 16, 2023
@alambalamb changed the title chore: break earlier in marco containschore: break earlier in macro contains!Jul 17, 2023

@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 makes sense to me. THank you @Weijun-H

It possible I think it would be great if @jayzhan211 or @izveigor could give it a look as well

@izveigor

Copy link
Copy Markdown
Contributor

@alamb I don't think this feature makes sense now since I and @jayzhan211 have decided to rewrite array_contains function.
This implementation is carried out by @jayzhan211. So I want to hear his opinion about this PR.

@jayzhan211

Copy link
Copy Markdown
Contributor

Actually, I did similar improvements in my array_has implementation #6990 before I know this PR. However, merging this PR is fine with me.

@izveigorizveigor 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.

Agree with @jayzhan211. LGTM!

@alamb
alamb merged commit 9338880 into apache:mainJul 17, 2023
@alamb

Copy link
Copy Markdown
Contributor

Thanks everyone

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

Labels

physical-exprChanges to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Weijun-H@izveigor@jayzhan211@alamb