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.

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

[native]: Add operator override for xxhash64, combine_hash internal functions #24503

Merged
merged 1 commit into from
Feb 6, 2025
Merged

[native]: Add operator override for xxhash64, combine_hash internal functions #24503

merged 1 commit into from
Feb 6, 2025

Conversation

pradeepvaka
Copy link
Contributor

@pradeepvaka pradeepvaka commented Feb 5, 2025 •
edited
Loading

Summary: Add operator override for xxhash64 and combine_hash internal operators to unblock join prefilter optimization for Presto native execution.

Differential Revision: D68917161

@pradeepvaka pradeepvaka requested a review from a team as a code owner February 5, 2025 19:24
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Feb 5, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: pradeepvaka / name: Pradeep Vaka (660432e)

@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D68917161

@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D68917161

@aditi-pandit aditi-pandit changed the title Add operator override for xxhash64, combine_hash internal functions native: Add operator override for xxhash64, combine_hash internal functions Feb 6, 2025
aditi-pandit
aditi-pandit approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @pradeepvaka

@aditi-pandit aditi-pandit changed the title native: Add operator override for xxhash64, combine_hash internal functions [native]: Add operator override for xxhash64, combine_hash internal functions Feb 6, 2025
@aditi-pandit aditi-pandit merged commit 391541a into prestodb:master Feb 6, 2025
63 checks passed
@aditi-pandit
Copy link
Contributor

@pradeepvaka : I approved and merged this PR. But I presume you have a bigger PR showing the e2e optimization in native engine. Is that ready yet ?

@pradeepvaka
Copy link
Contributor Author

@aditi-pandit thanks for approval. Join prefilter optimization already exists in Presto but the xxhash_64 and combine_hash internal operators were missing on native. These internal operators were added to velox and the overrides are updated in the commit for e2e to work.

https://github.com/prestodb/presto/blob/391541a831bdcd0b65d3ebb361b8324019977f0b/presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/JoinPrefilter.java#L116C1-L132C4

@aditi-pandit
Copy link
Contributor

@pradeepvaka : Got you. Can you add some tests in https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeJoinQueries.java that will set and test "JOIN_PREFILTER_BUILD_SIDE" system property ?

This was referenced Mar 10, 2025
Closed
Closed
jp-sivaprasad pushed a commit to jp-sivaprasad/presto that referenced this pull request Mar 10, 2025
@pradeepvaka @jp-sivaprasad
Add operator override for xxhash64, combine_hash internal functions (p…
231dfdb 
…restodb#24503)

Summary:

TSIA

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

@aditi-pandit aditi-pandit aditi-pandit approved these changes

Assignees
No one assigned
Labels
fb-exported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pradeepvaka @facebook-github-bot @aditi-pandit