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

Rearrange logical optimizing rule #55231

Open
2 of 25 tasks
AilinKid opened this issue Aug 6, 2024 · 10 comments
Open
2 of 25 tasks

Rearrange logical optimizing rule #55231

AilinKid opened this issue Aug 6, 2024 · 10 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement.

Comments

@AilinKid
Copy link
Contributor

AilinKid commented Aug 6, 2024

Enhancement

This is a special topic campaign about first-time contributions that are friendly for academic open-source credits, with no hard-core database logic inside. Still, it is a great window for stepping deep into the charm of TiDB optimizer. Here is an example, #55226, this pull request contain the interface refactoring, you should focus on the change of rule_build_key_info.go about moving and classifying current logical rules.

currently, tidb optimizing logical rules are put in the planner/core pkg with the prefix rule_, inside these files it may rely on logical operator pkg to do the type check and rule asserting. Some of it may have little coupling with planner/core pkg itselves. When we try to move and classify these rules into planner/rule pkg, we should think twice about how to solve these back awards dependencies. The healthy pkg reference graph should be like below:

image

some special cases'(complex rule file migrating) scope will become even bigger than the pictures show, you can contact me @AilinKid and @hawkingrei to talk about it.

You are appreciated for making a detailed description of what your pull request has done. Of course,

  • first, you should claim and comment on which rule_file you picked behind this issue, click the checkbox below at the same time,
  • then, after you go through the code, you can modify the same comment to add what's your basic idea about it.
  • after @AilinKid and @hawkingrei approve your idea, you can file your pull request. This is to avoid wasting time on the wrong way too much, describing your idea is much cheaper, isn't it? Thank you so much for reading until here.
  • rule_aggregation_elimination.go
  • rule_aggregation_push_down.go
  • rule_aggregation_skew_rewrite.go
  • rule_collect_plan_stats.go @hawkingrei
  • rule_column_pruning.go
  • rule_constant_propagation.go @qingfeng777
  • rule_decorrelate.go
  • rule_derive_topn_from_window.go
  • rule_eliminate_projection.go
  • rule_generate_column_substitute.go
  • rule_inject_extra_projection.go
  • rule_join_elimination.go
  • rule_join_reorder.go
  • rule_join_reorder_dp.go
  • rule_join_reorder_greedy.go
  • rule_max_min_eliminate.go
  • rule_outer_to_inner_join.go
  • rule_partition_processor.go
  • rule_predicate_push_down.go
  • rule_predicate_simplification.go
  • rule_push_down_sequence.go
  • rule_resolve_grouping_expand.go
  • rule_result_reorder.go
  • rule_semi_join_rewrite.go
  • rule_topn_push_down.go

First Time Contributor

first time contributor with tag in your pull request like below
image
After it merged, you can reach @billmay for your souvenir

@AilinKid AilinKid added type/enhancement The issue or PR belongs to an enhancement. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Aug 6, 2024
@hawkingrei hawkingrei added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Aug 6, 2024
@AilinKid AilinKid added the sig/planner SIG: Planner label Aug 8, 2024
@qingfeng777
Copy link
Contributor

Could you assign the rule_constant_propagation.go to me? I will move it later.
By the way, I don't know how to click the checkbox below at the same time, it's not clickable for me. Could you tell me how?

@AilinKid
Copy link
Contributor Author

Could you assign the rule_constant_propagation.go to me? I will move it later. By the way, I don't know how to click the checkbox below at the same time, it's not clickable for me. Could you tell me how?

welcome and thanks for participating, the checkbox is acked and it's assigned to you.

basically from the workflow, you should add your idea first in the comment then file your pull request after we acked.
like

"idea: after migrating the file, the function validCompareConstantPredicate is not valid anymore, it is refed by logical selection in the core pkg. I moved it into constant_propagation.go as in ruleutil for usage."

anyway, for this one these changes look good to me. thx again.

@qingfeng777
Copy link
Contributor

Regarding the rule_predicate_push_down.go file,

my suggestion is as follows:

After migrating, keep the functions appendDataSourcePredicatePushDownTraceStep and addExprPrefixCond in the original package since they depend on dataSource. Also, retain the exprPrefixAdder struct and its functions in the original package because they are referenced by the function addExprPrefixCond. The function addSelection should remain in the original file as well, given its dependency on the logicalop package.

Finally, rename the original file to expr_prefix_adder.go and move the dataSource functions to logical_datasource.go under /pkg/planner/core/.

@hawkingrei
Copy link
Member

hawkingrei commented Aug 14, 2024

We appreciate @qingfeng777's active contributions to TiDB, but we intend to reserve this issue for newcomers making their first contributions to TiDB, you are already admitted to TiDB contributors. Therefore, how about sparing some chances for the other enthusiasts?
image

@qingfeng777
Copy link
Contributor

The last one is too easy, so I tried another one. Anyway, it's OK.

@Risc-lt
Copy link

Risc-lt commented Sep 3, 2024

Could you assign the rule_predicate_push_down.go to me?
BTW, since I'm first here, the workflow is not very clear for me. Could you give me some guidance on where to start?

@hawkingrei
Copy link
Member

Could you assign the rule_predicate_push_down.go to me? BTW, since I'm first here, the workflow is not very clear for me. Could you give me some guidance on where to start?

Yes, Welcome.

@hawkingrei
Copy link
Member

The last one is too easy, so I tried another one. Anyway, it's OK.

Thank you.

@orthur2
Copy link

orthur2 commented Oct 1, 2024

I hope this message finds you well. I would like to take ownership of the task related to the rule_topn_push_down.go function. Since this is my first time participating in the project, I am somewhat unfamiliar with its structure.Please let me know if there is any mistake.I would appreciate it.

My plan is to create a new file rule_topn_push_down.go in the planner/rule directory and move the PushDownTopNOptimizer along with the related functions (Optimize, pushDownTopNForBaseLogicalPlan, and Name) from planner/core to this new file.

I would appreciate any suggestions or guidance you may have, as I want to ensure I follow the project's conventions correctly. Thank you for your support.

@orthur2
Copy link

orthur2 commented Oct 11, 2024

@AilinKid @hawkingrei I'm sorry if this disturbed you. I'm writing to follow up on my previous comment regarding the rule_topn_push_down.go task, as it has been ten days since I expressed my interest. I'm commenting again,not as a complaint but as unsure whether the previous comment was seen. Thank you for your attention, and looking forward to your response.

@hawkingrei hawkingrei pinned this issue Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

No branches or pull requests

5 participants