planner: complete the memo/group-expression/group logic and add test for derive stats #57973

AilinKid · 2024-12-04T08:11:34Z

What problem does this PR solve?

Issue Number: ref #51664

Problem Summary:

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

tiprow · 2024-12-04T08:11:51Z

Hi @AilinKid. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

AilinKid · 2024-12-04T08:13:35Z

pkg/planner/cascades/memo/memo.go

 	// groupIDGen is the incremental group id for internal usage.
-	groupIDGen GroupIDGenerator
+	groupIDGen *GroupIDGenerator


according the detail design, memo/rule should be not aware of session context, but scheduler and task did. so here remove it.

codecov · 2024-12-04T08:41:56Z

Codecov Report

Attention: Patch coverage is 76.19048% with 25 lines in your changes missing coverage. Please review.

Project coverage is 73.5577%. Comparing base (0ffac36) to head (d64a273).
Report is 16 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57973        +/-   ##
================================================
+ Coverage   73.1496%   73.5577%   +0.4081%     
================================================
  Files          1675       1676         +1     
  Lines        461848     463863      +2015     
================================================
+ Hits         337840     341207      +3367     
+ Misses       103258     101832      -1426     
- Partials      20750      20824        +74

Flag	Coverage Δ
integration	`43.1836% <0.0000%> (?)`
unit	`72.3632% <76.1904%> (+0.0822%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.5145% <ø> (-0.4718%)`	⬇️

winoros · 2024-12-04T17:48:59Z

pkg/planner/core/casetest/cascades/testdata/cascades_suite_out.json

+    "Cases": [
+      {
+        "SQL": "select count(1) from t1 group by a, b",
+        "Str": "GID:1, logic prop:{stats:{count 5, ColNDVs map[1:2 2:2]}, schema:{Column: [test.t1.a,test.t1.b] Key: [] Unique key: []}}\nGID:2, logic prop:{stats:{count 4, ColNDVs map[4:4]}, schema:{Column: [Column#4] Key: [] Unique key: []}}\nGID:3, logic prop:{stats:{count 4, ColNDVs map[4:4]}, schema:{Column: [Column#4] Key: [] Unique key: []}}"


The \n seems to be parsed in the wrong way?

json string seems couldn't switch line obviously, i change it to string slice, which will be much more clear

AilinKid · 2024-12-06T07:45:25Z

/retest-required

tiprow · 2024-12-06T07:45:49Z

@AilinKid: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest-required

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

AilinKid · 2024-12-06T07:46:13Z

/ok-to-test

Signed-off-by: arenatlx <314806019@qq.com>

pkg/planner/cascades/memo/group_expr.go

Signed-off-by: arenatlx <314806019@qq.com>

hawkingrei · 2024-12-10T05:23:02Z

/retest

Signed-off-by: arenatlx <314806019@qq.com>

pkg/planner/cascades/memo/group.go

Signed-off-by: arenatlx <314806019@qq.com>

AilinKid · 2024-12-11T06:36:34Z

/test unit-test

tiprow · 2024-12-11T06:36:55Z

@AilinKid: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test unit-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Signed-off-by: arenatlx <314806019@qq.com>

AilinKid · 2024-12-11T06:58:38Z

/retest-required

hawkingrei · 2024-12-11T07:25:07Z

/retest

Signed-off-by: arenatlx <314806019@qq.com>

tiprow · 2024-12-11T09:36:25Z

@AilinKid: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	`d64a273`	link	true	`/test fast_test_tiprow`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

ti-chi-bot · 2024-12-11T09:37:16Z

@hawkingrei: Your lgtm message is repeated, so it is ignored.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ti-chi-bot · 2024-12-11T09:44:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawkingrei, winoros

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/planner/OWNERS~~ [hawkingrei,winoros]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-12-11T09:44:12Z

[LGTM Timeline notifier]

Timeline:

2024-12-11 07:33:51.527094342 +0000 UTC m=+423821.615896885: ☑️ agreed by hawkingrei.
2024-12-11 09:44:10.976777376 +0000 UTC m=+431641.065579917: ☑️ agreed by winoros.

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 4, 2024

AilinKid commented Dec 4, 2024

View reviewed changes

winoros reviewed Dec 4, 2024

View reviewed changes

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Dec 6, 2024

AilinKid added 12 commits December 9, 2024 20:10

.

ab82ca5

Signed-off-by: arenatlx <314806019@qq.com>

.

5e1234e

Signed-off-by: arenatlx <314806019@qq.com>

.

a64accf

Signed-off-by: arenatlx <314806019@qq.com>

.

07bdc92

Signed-off-by: arenatlx <314806019@qq.com>

.

7d3e588

Signed-off-by: arenatlx <314806019@qq.com>

.

45b2630

Signed-off-by: arenatlx <314806019@qq.com>

.

a6ff6ff

Signed-off-by: arenatlx <314806019@qq.com>

.

f9279e5

Signed-off-by: arenatlx <314806019@qq.com>

.

23eae91

Signed-off-by: arenatlx <314806019@qq.com>

.

f68716a

Signed-off-by: arenatlx <314806019@qq.com>

.

6b390fa

Signed-off-by: arenatlx <314806019@qq.com>

.

4125681

Signed-off-by: arenatlx <314806019@qq.com>

AilinKid force-pushed the refine-mm branch from 9e7c864 to 4125681 Compare December 9, 2024 12:23

AilinKid added 2 commits December 10, 2024 12:34

.

c3fca98

Signed-off-by: arenatlx <314806019@qq.com>

.

0c55517

Signed-off-by: arenatlx <314806019@qq.com>

hawkingrei reviewed Dec 10, 2024

View reviewed changes

pkg/planner/cascades/memo/group_expr.go Outdated Show resolved Hide resolved

.

2418df4

Signed-off-by: arenatlx <314806019@qq.com>

.

3714055

Signed-off-by: arenatlx <314806019@qq.com>

AilinKid added 5 commits December 10, 2024 15:38

fix old test

4cbc1e7

Signed-off-by: arenatlx <314806019@qq.com>

eliminate the unnecessary tmp variable

1590021

Signed-off-by: arenatlx <314806019@qq.com>

.

e4d594f

Signed-off-by: arenatlx <314806019@qq.com>

.

5910596

Signed-off-by: arenatlx <314806019@qq.com>

.

32d2325

Signed-off-by: arenatlx <314806019@qq.com>

hawkingrei reviewed Dec 11, 2024

View reviewed changes

pkg/planner/cascades/memo/group.go Outdated Show resolved Hide resolved

.

fab10f0

Signed-off-by: arenatlx <314806019@qq.com>

.

6316c2b

Signed-off-by: arenatlx <314806019@qq.com>

hawkingrei approved these changes Dec 11, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Dec 11, 2024

AilinKid added 2 commits December 11, 2024 17:29

intro failpoint

365588a

Signed-off-by: arenatlx <314806019@qq.com>

.

d64a273

Signed-off-by: arenatlx <314806019@qq.com>

ti-chi-bot bot added the approved label Dec 11, 2024

hawkingrei approved these changes Dec 11, 2024

View reviewed changes

winoros approved these changes Dec 11, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 11, 2024

ti-chi-bot bot merged commit ba55be5 into pingcap:master Dec 11, 2024
23 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: complete the memo/group-expression/group logic and add test for derive stats #57973

planner: complete the memo/group-expression/group logic and add test for derive stats #57973

AilinKid commented Dec 4, 2024

tiprow bot commented Dec 4, 2024

AilinKid Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

winoros Dec 4, 2024

AilinKid Dec 6, 2024 •

edited

Loading

AilinKid commented Dec 6, 2024

tiprow bot commented Dec 6, 2024

AilinKid commented Dec 6, 2024

hawkingrei commented Dec 10, 2024

AilinKid commented Dec 11, 2024

tiprow bot commented Dec 11, 2024

AilinKid commented Dec 11, 2024

hawkingrei commented Dec 11, 2024

tiprow bot commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

planner: complete the memo/group-expression/group logic and add test for derive stats #57973

planner: complete the memo/group-expression/group logic and add test for derive stats #57973

Conversation

AilinKid commented Dec 4, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Dec 4, 2024

AilinKid Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

winoros Dec 4, 2024

Choose a reason for hiding this comment

AilinKid Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

AilinKid commented Dec 6, 2024

tiprow bot commented Dec 6, 2024

AilinKid commented Dec 6, 2024

hawkingrei commented Dec 10, 2024

AilinKid commented Dec 11, 2024

tiprow bot commented Dec 11, 2024

AilinKid commented Dec 11, 2024

hawkingrei commented Dec 11, 2024

tiprow bot commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

[LGTM Timeline notifier]

AilinKid Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

AilinKid Dec 6, 2024 •

edited

Loading