Skip to content

Support dynamic filter for hash join - #15763

Open
SilenceJD wants to merge 1 commit into
prestodb:release-0.248from
SilenceJD:hash-df
Open

Support dynamic filter for hash join #15763
SilenceJD wants to merge 1 commit into
prestodb:release-0.248from
SilenceJD:hash-df

Conversation

@SilenceJD

Copy link
Copy Markdown
Contributor

Support dynamic filter for hash join. Currently, Presto 0.241+ supports local dynamic filtering for broadcast inner-joins, but does not have a dynamic filtering for hash inner-joins. This PR add a new session property "ENABLE_HASH_JOIN_DYNAMIC_FILTERING" which support dynamic filter for hash join.

== RELEASE NOTES ==

General Changes

* Add `HashJoinDynamicFilterSourceOperator` to collect filter data info before hash table build in build side.

* Add `DynamicFilterService` to aggregate filter data info in coordinator and broadcast it to the probe side.

* Add `DynamicPageFilter` to do page filtering in the probe-side by filter data info which receive from the build side.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Mar 2, 2021

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

@SilenceJD
SilenceJD force-pushed the hash-df branch 2 times, most recently from 14b080a to d88b8aa Compare March 3, 2021 08:13

@highker highker left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Haven't looked into design yet. Maybe let's fix the coding style first?

Comment thread pom.xml Outdated
Comment on lines 271 to 275

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we use airlift json?

Comment on lines 176 to 177

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We don't use any abbreviations or acronyms in our code base. Please spell out bf. Also, there are a lot of other places in this PR having this issue. Could you go over them and fix accordingly?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"bloom" is a weird name. Use "bloomfilter"

@SilenceJD

Copy link
Copy Markdown
Contributor Author

Haven't looked into design yet. Maybe let's fix the coding style first?

Sure, I will fix them right away.

@SilenceJD
SilenceJD force-pushed the hash-df branch 5 times, most recently from 8950c98 to e4f2c33 Compare March 8, 2021 10:16
@rohanpednekar

Copy link
Copy Markdown
Contributor

Gentle Ping! 🙏

@highker

highker commented Oct 5, 2021

Copy link
Copy Markdown

@kewang1024, could you help to review this PR?

@rubenssoto

Copy link
Copy Markdown

Its is a great contribution. only a gentle reminder. :)

@yingsu00

Copy link
Copy Markdown
Contributor

Is this still being worked on? @SilenceJD

@rohanpednekar

Copy link
Copy Markdown
Contributor

@SilenceJD Hope all is well with you. Just checking in to see if you are still planning to work on this PR?
@v-jizhang Do you think you can pick this up further to get it merged if needed? Thanks!

@SilenceJD

Copy link
Copy Markdown
Contributor Author

Is this still being worked on? @SilenceJD

@yingsu00 yes, I'd be happy to follow up on this PR if community can give some feedback and suggestions.

@SilenceJD

Copy link
Copy Markdown
Contributor Author

@SilenceJD Hope all is well with you. Just checking in to see if you are still planning to work on this PR? @v-jizhang Do you think you can pick this up further to get it merged if needed? Thanks!

@rohanpednekar Thank you for your concern. I can continue to work for this request, and I'm glad you and your partners can join in.

@rohanpednekar

Copy link
Copy Markdown
Contributor

Thanks @SilenceJD.

@yingsu00, Do you think you can further review this PR and provide your feedback for @SilenceJD ?

@cliandy

cliandy commented Aug 29, 2022

Copy link
Copy Markdown
Contributor

@SilenceJD @rohanpednekar do we have any updates on this? This would be very helpful for us and would be happy to work on this to get it out asap.

@lfyzjck

lfyzjck commented May 11, 2023

Copy link
Copy Markdown

We had the same problem, which cause prestodb had bad perf in tpcds q17 compare with trino

@rohanpednekar

Copy link
Copy Markdown
Contributor

@atris Ses if you can pick this one?

@tdcmeehan

Copy link
Copy Markdown
Contributor

@SilenceJD it looks like this slipped between the cracks. Nonetheless, this would be a great feature, and could significantly improve TPC-DS performance, so I'm hoping you could help us pick this back up. We would like to help you push this forward--are you prepared to address review feedback and help move it forward? If so, we can review and help.

@SilenceJD SilenceJD closed this Mar 21, 2024
@SilenceJD SilenceJD reopened this Mar 21, 2024
@SilenceJD

Copy link
Copy Markdown
Contributor Author

@SilenceJD it looks like this slipped between the cracks. Nonetheless, this would be a great feature, and could significantly improve TPC-DS performance, so I'm hoping you could help us pick this back up. We would like to help you push this forward--are you prepared to address review feedback and help move it forward? If so, we can review and help.

@tdcmeehan Thank you very much for your reply, I am glad that you can help and start this work again. But this pr is already far behind the master branch, I may need to resubmit a new pr based on the master branch and improve some of the previous implementations. Please give me some time~

@tdcmeehan

Copy link
Copy Markdown
Contributor

@SilenceJD that's wonderful--we'll be ready to look once you've rebased and are ready for review. Thanks a lot!

@sajjoseph

Copy link
Copy Markdown

Is this still being working on?
@SilenceJD - hope you can bring this to a closure.
Thanks!

@tdcmeehan

Copy link
Copy Markdown
Contributor

@sajjoseph I'm working on this, see prestodb/rfcs#54

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

Projects

Status: 🆕 Unprioritized

Development

Successfully merging this pull request may close these issues.

10 participants