Skip to content
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.

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

Refactor AggregateCompanionAdapter to remove duplicate code #9920

Closed

Conversation

wypb
Copy link
Contributor

@wypb wypb commented May 24, 2024

Refactor AggregateCompanionAdapter to remove duplicate code.

CC: @kagamiori @mbasmanova

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 24, 2024
Copy link

netlify bot commented May 24, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 608beaf
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/665eadc467bf630008ee0761

@wypb wypb force-pushed the refactor_AggregateCompanionAdapter branch from 7e4ee37 to db35de2 Compare May 24, 2024 03:09
@mbasmanova mbasmanova requested a review from kagamiori May 29, 2024 12:24
Copy link
Contributor

@kagamiori kagamiori left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the refactoring!

@facebook-github-bot
Copy link
Contributor

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@kagamiori kagamiori left a comment

Choose a reason for hiding this comment

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

Hi @wypb, could you please rebase this PR onto the latest main? Thanks!

registered |= exec::registerStatefulVectorFunction(
CompanionSignatures::extractFunctionNameWithSuffix(originalName, type),
extractSignatures,
factory,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: std::move(extractSignatures) and std::move(factory). Ditto for lines 503--504.

Copy link
Contributor Author

@wypb wypb Jun 3, 2024

Choose a reason for hiding this comment

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

Hi @kagamiori Thank you for your reply. I've reconstructed it and synchronized the latest code.

@wypb wypb force-pushed the refactor_AggregateCompanionAdapter branch 2 times, most recently from 977ff97 to fe8c99c Compare June 3, 2024 23:56
@facebook-github-bot
Copy link
Contributor

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@wypb wypb force-pushed the refactor_AggregateCompanionAdapter branch from fe8c99c to 608beaf Compare June 4, 2024 06:01
@facebook-github-bot
Copy link
Contributor

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kagamiori merged this pull request in 3aebeb6.

Copy link

Conbench analyzed the 1 benchmark run on commit 3aebeb66.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@wypb wypb deleted the refactor_AggregateCompanionAdapter branch June 5, 2024 06:17
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
…incubator#9920)

Summary:
Refactor AggregateCompanionAdapter to remove duplicate code.

CC: kagamiori mbasmanova

Pull Request resolved: facebookincubator#9920

Reviewed By: pedroerp

Differential Revision: D58087248

Pulled By: kagamiori

fbshipit-source-id: e9b726eed5cf39b45470c8938143d5a112149789
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
…incubator#9920)

Summary:
Refactor AggregateCompanionAdapter to remove duplicate code.

CC: kagamiori mbasmanova

Pull Request resolved: facebookincubator#9920

Reviewed By: pedroerp

Differential Revision: D58087248

Pulled By: kagamiori

fbshipit-source-id: e9b726eed5cf39b45470c8938143d5a112149789
deepashreeraghu pushed a commit to deepashreeraghu/velox that referenced this pull request Jun 13, 2024
…incubator#9920)

Summary:
Refactor AggregateCompanionAdapter to remove duplicate code.

CC: kagamiori mbasmanova

Pull Request resolved: facebookincubator#9920

Reviewed By: pedroerp

Differential Revision: D58087248

Pulled By: kagamiori

fbshipit-source-id: e9b726eed5cf39b45470c8938143d5a112149789
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants