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

migrate test-infra to testify for planner/core/rule_join_reorder_dp_test.go #28397

Closed
Tracked by #26857
tisonkun opened this issue Sep 27, 2021 · 4 comments · Fixed by #32179
Closed
Tracked by #26857

migrate test-infra to testify for planner/core/rule_join_reorder_dp_test.go #28397

tisonkun opened this issue Sep 27, 2021 · 4 comments · Fixed by #32179
Assignees
@johnhaxx7 @tisonkun
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. type/enhancement The issue or PR belongs to an enhancement.

Comments

@tisonkun
Copy link
Contributor

No description provided.

@tisonkun tisonkun mentioned this issue Sep 27, 2021
Closed
30 tasks
@tisonkun tisonkun added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Sep 27, 2021
@johnhaxx7
Copy link
Contributor

/assign

@johnhaxx7
Copy link
Contributor

Hi @tisonkun, to migrate this file,
s *testJoinReorderDPSuite in function newMockJoin will be removed, which will cause
s.ctx at line 61 and s.statsMap at line 63 is not able to pass to this function. Please check details from here:

tidb/planner/core/rule_join_reorder_dp_test.go

Line 61 in ad94300

retJoin := mockLogicalJoin{}.init(s.ctx)

In this case,

tidb/planner/core/rule_join_reorder_dp.go

Line 26 in 6b1010f

newJoin func(lChild, rChild LogicalPlan, eqConds []*expression.ScalarFunction, otherConds []expression.Expression) LogicalPlan
might needs to be refactored to pass in the context and statsMap. Is it ok to do so or do you have a better idea?

@johnhaxx7
Copy link
Contributor

johnhaxx7 commented Feb 5, 2022 •
edited
Loading

+ @hawkingrei for more visibility

@tisonkun tisonkun added the type/enhancement The issue or PR belongs to an enhancement. label Feb 9, 2022
@tisonkun
Copy link
Contributor Author

tisonkun commented Feb 9, 2022

@johnhaxx7 thanks for working on this issue. For the specific problem, I'll directly submit a PR for this issue so that it's explained by code. You can help on the review side then.

@tisonkun tisonkun self-assigned this Feb 9, 2022
@tisonkun tisonkun mentioned this issue Feb 9, 2022
Merged
ti-chi-bot pushed a commit that referenced this issue Feb 9, 2022
@tisonkun
planner/core: migrate test-infra to testify for rule_ tests (#32179)
297455d 
close #28396, close #28397, close #28398
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@johnhaxx7 johnhaxx7

@tisonkun tisonkun

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. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

planner/core: migrate test-infra to testify for rule_ tests tisonkun/tidb
2 participants
@johnhaxx7 @tisonkun