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

[CALCITE-6846] Support basic dphyp join reorder algorithm #4204

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

silundong
Copy link

@silundong silundong commented Feb 19, 2025

The main enumeration process of dphyp has been implemented. There are two new rules, one for converting Joins into a HyperGraph and the other for triggering the dphyp algorithm. More details could see:https://issues.apache.org/jira/browse/CALCITE-6846

@silundong silundong closed this Feb 19, 2025
@silundong silundong deleted the dphyp branch February 19, 2025 11:49
@silundong silundong restored the dphyp branch February 19, 2025 11:52
@silundong silundong reopened this Feb 19, 2025
@silundong silundong force-pushed the dphyp branch 3 times, most recently from 2e00af5 to 658371b Compare February 20, 2025 10:52
@mihaibudiu
Copy link
Contributor

Is this ready for review?
If not, please mark as draft.

@silundong
Copy link
Author

Is this ready for review? If not, please mark as draft.

Ready, please review.
There are still some checks that are not passed, but I think I have taken care of them in the code. Can you give me some advice on how to pass these checks?

@NobiGo
Copy link
Contributor

NobiGo commented Feb 21, 2025

Some parameters might need to be checked whether they are NULL or undergo active type conversion.

@silundong
Copy link
Author

Some parameters might need to be checked whether they are NULL or undergo active type conversion.

Thank you, I will update it.

@rubenada
Copy link
Contributor

This is a great contribution @silundong.
Just out of curiosity, have you tested locally what happens if you add the new HyperGraph rules into the "default rules" (e.g. RelOptRules#BASE_RULES, or Programs#RULE_SET) and execute Calcite tests? Do you get any any suspicious error (except for the expected plan changes regarding Join-ordering that the new rules may generate)?

@silundong
Copy link
Author

This is a great contribution @silundong. Just out of curiosity, have you tested locally what happens if you add the new HyperGraph rules into the "default rules" (e.g. RelOptRules#BASE_RULES, or Programs#RULE_SET) and execute Calcite tests? Do you get any any suspicious error (except for the expected plan changes regarding Join-ordering that the new rules may generate)?

Thank you very much for your recognition. I did not add it to the "default rules" (e.g. RelOptRules#BASE_RULES, or Programs#RULE_SET). However, in other projects, I have integrated the Calcite parsing and optimization module and added HyperGraph rules. It has worked without exceptions under TPC-DS.

@silundong silundong force-pushed the dphyp branch 2 times, most recently from df60b87 to 6aa88b2 Compare February 26, 2025 09:28
@silundong silundong requested a review from rubenada February 26, 2025 10:50
@rubenada
Copy link
Contributor

Thanks for addressing my initial comments @silundong , I'll take another look in the coming days.

@silundong
Copy link
Author

@rubenada Thank you very much for your review!

@rubenada
Copy link
Contributor

rubenada commented Mar 4, 2025

@silundong I have left some remarks , rather generic or Calcite-related, not really dphyp-algo related, which I find I bit hard to follow, but it's good that you provided tests showing the results.
IMO we could consider adding this feature, maybe initially flagging the new classes and rule definitions as @Experimental (as it happened in the past with other features). Let's see what other contributors think about it...

@silundong
Copy link
Author

silundong commented Mar 5, 2025

@silundong I have left some remarks , rather generic or Calcite-related, not really dphyp-algo related, which I find I bit hard to follow, but it's good that you provided tests showing the results. IMO we could consider adding this feature, maybe initially flagging the new classes and rule definitions as @Experimental (as it happened in the past with other features). Let's see what other contributors think about it...

Thank you very much for your suggestion, I will update them. I fully support adding the @Experimental annotation. It is a new feature that needs to be validated through more extensive use. In the early stages, users can use it selectively.

@silundong silundong requested a review from rubenada March 7, 2025 02:21
@rubenada
Copy link
Contributor

rubenada commented Mar 8, 2025

@silundong could you please take a look at Alessandro's minor comments in the Jira ticket?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants