Skip to content

minor: remove custom extract_ok! macro - #14733

Merged
alamb merged 1 commit into
apache:mainfrom
ctsk:remove-extract-ok
Feb 19, 2025
Merged

minor: remove custom extract_ok! macro#14733
alamb merged 1 commit into
apache:mainfrom
ctsk:remove-extract-ok

Conversation

@ctsk

@ctskctsk commented Feb 17, 2025

Copy link
Copy Markdown
Contributor

When reading through aggregation code, I saw that the extract_ok! macro seems to do the same thing as the ? operator in this context.

/// Extracts a successful Ok(_) or returns Poll::Ready(Some(Err(e))) with errorsmacro_rules! extract_ok {($RES: expr) => {{match $RES {Ok(v) => v,Err(e) => returnPoll::Ready(Some(Err(e))),}}};}

I've replaced the usages of extract_ok with ? in this PR and removed the macro.

@github-actionsgithub-actionsBot added the physical-expr Changes to the physical-expr crates label Feb 17, 2025
@ctsk

ctsk commented Feb 17, 2025

Copy link
Copy Markdown
ContributorAuthor

@alamb I see that you've previously attempted something similar in #7025 - but a larger refactoring is not (no longer?) necessary to drop extract_ok.

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

Makes sense to me -- thank you @ctsk@mbrobbel and @xudong963
I think maybe when this was first written Poll didn't support the ? operator (or maybe I just didn't understand it)

@alamb
alamb merged commit ee2d2a4 into apache:mainFeb 19, 2025
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

@ctsk@alamb@mbrobbel@xudong963