planner: fix the issue that UnionAll didn't handle the range bump case #52542

YangKeao · 2024-04-12T06:02:19Z

What problem does this PR solve?

Issue Number: close #52472

Problem Summary:

The UnionAll should use types.AggFieldType to aggregate the fieldTypes. This function will handle the bump of range. For example, the aggregate of signed and unsigned long will give signed long long.

What changed and how does it work?

Use types.AggFieldType instead of types.MergeFieldType.

This PR also makes types.MergeFieldType private, as it's easy to make such mistakes.

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.

Fix the issue that the content of the result of `union all` may overflow the type.

codecov · 2024-04-12T07:08:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.2709%. Comparing base (0362dc8) to head (c0eb773).
Report is 176 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #52542        +/-   ##
================================================
+ Coverage   72.1924%   77.2709%   +5.0785%     
================================================
  Files          1470       1518        +48     
  Lines        427189     470877     +43688     
================================================
+ Hits         308398     363851     +55453     
+ Misses        99588      87223     -12365     
- Partials      19203      19803       +600

Flag	Coverage Δ
integration	`52.6536% <100.0000%> (?)`
unit	`73.8066% <100.0000%> (+2.7281%)`	⬆️

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

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.1073% <ø> (+11.0206%)`	⬆️

YangKeao · 2024-04-12T07:42:16Z

tests/integrationtest/r/explain_easy.result

@@ -516,7 +516,7 @@ StreamAgg	1.00	root		funcs:count(1)->Column#22
      │   └─Projection	1.00	root		Column#33, Column#34, Column#35, Column#14, Column#15, Column#16
      │     └─HashAgg	1.00	root		group by:explain_easy.test01.region_id, explain_easy.test01.show_date, explain_easy.test01.stat_date, funcs:firstrow(explain_easy.test01.stat_date)->Column#33, funcs:firstrow(explain_easy.test01.show_date)->Column#34, funcs:firstrow(explain_easy.test01.region_id)->Column#35, funcs:firstrow(explain_easy.test01.stat_date)->Column#14, funcs:firstrow(explain_easy.test01.show_date)->Column#15, funcs:firstrow(explain_easy.test01.region_id)->Column#16, funcs:count(1)->Column#39
      │       └─TableReader	0.01	root		data:Selection
-      │         └─Selection	0.01	cop[tikv]		eq(explain_easy.test01.period, 1), ge(explain_easy.test01.stat_date, 20191202), gt(cast(explain_easy.test01.registration_num, bigint(20) BINARY), 0), le(explain_easy.test01.stat_date, 20191202)
+      │         └─Selection	0.01	cop[tikv]		eq(explain_easy.test01.period, 1), ge(explain_easy.test01.stat_date, 20191202), gt(cast(explain_easy.test01.registration_num, decimal(20,0) BINARY), 0), le(explain_easy.test01.stat_date, 20191202)


It unions integer literal 0 with the unsigned big int. It's expected to produce new decimal. I've checked that this behavior is compatible with MySQL, and the related tests have been added in TestIssue52472

YangKeao · 2024-04-15T10:16:06Z

pkg/planner/core/casetest/hint/testdata/integration_suite_out.json

+          "              ├─Selection 3323.33 mpp[tiflash]  lt(test.t.a, 18), lt(test.t.a, 60)]",
+          "              │ └─TableFullScan 10000.00 mpp[tiflash] table:t pushed down filter:empty, keep order:false, stats:pseudo]",
+          "              └─Selection 1107.78 mpp[tiflash]  gt(test.t.b, 1), lt(test.t.a, 60)]",
+          "                └─TableFullScan 10000.00 mpp[tiflash] table:t pushed down filter:empty, keep order:false, stats:pseudo]",


It's expected. The original column definition of Union doesn't have NOT NULL flag, which is the reason for the cast above.

The mergeFieldType didn't handle the type flag, but the AggFieldType did. In this PR, both the original column in table and the union column have the NOT NULL flag, so the cast is not needed.

Signed-off-by: YangKeao <[email protected]>

YangKeao · 2024-04-15T11:46:52Z

/retest

time-and-fate

LGTM.
But there's more difference between the new and old logic than the bugfix itself. I'm not sure if they are all expected and correct.
I think it's better to have another reviewer more familiar with the types.

ti-chi-bot · 2024-05-11T05:38:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: time-and-fate, xhebox

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:

~~OWNERS~~ [time-and-fate,xhebox]
~~pkg/planner/OWNERS~~ [time-and-fate]

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-05-11T05:38:42Z

[LGTM Timeline notifier]

Timeline:

2024-04-16 07:58:26.768149298 +0000 UTC m=+324925.879195744: ☑️ agreed by time-and-fate.
2024-05-11 05:38:41.32870209 +0000 UTC m=+1286075.085837662: ☑️ agreed by xhebox.

ti-chi-bot · 2024-05-13T03:50:03Z

In response to a cherrypick label: new pull request created to branch release-8.1: #53209.

ti-chi-bot · 2024-05-13T03:50:20Z

In response to a cherrypick label: new pull request created to branch release-6.5: #53210.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-05-13T03:50:22Z

In response to a cherrypick label: new pull request created to branch release-7.5: #53211.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-05-13T03:50:27Z

In response to a cherrypick label: new pull request created to branch release-7.1: #53212.

#52542) (#53209) close #52472

pingcap#52542) close pingcap#52472

#52542) (#53211) close #52472

ti-chi-bot bot added do-not-merge/needs-triage-completed do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 12, 2024

YangKeao marked this pull request as draft April 12, 2024 06:03

ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 12, 2024

YangKeao force-pushed the fix-52472 branch from bfe24cb to 1143223 Compare April 12, 2024 06:39

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed labels Apr 12, 2024

YangKeao marked this pull request as ready for review April 12, 2024 06:42

ti-chi-bot bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-tests-checked labels Apr 12, 2024

YangKeao force-pushed the fix-52472 branch from 1143223 to e6606b2 Compare April 12, 2024 07:37

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 12, 2024

YangKeao force-pushed the fix-52472 branch from e6606b2 to 3383c5e Compare April 12, 2024 07:41

YangKeao commented Apr 12, 2024

View reviewed changes

YangKeao force-pushed the fix-52472 branch 2 times, most recently from 2cfca00 to 8853551 Compare April 15, 2024 10:13

YangKeao commented Apr 15, 2024

View reviewed changes

YangKeao requested review from time-and-fate and hawkingrei April 15, 2024 10:16

YangKeao force-pushed the fix-52472 branch from 8853551 to 6ae9562 Compare April 15, 2024 10:35

fix the issue that UnionAll didn't handle the range bump case

c0eb773

Signed-off-by: YangKeao <[email protected]>

YangKeao force-pushed the fix-52472 branch from 6ae9562 to c0eb773 Compare April 15, 2024 11:34

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 15, 2024

time-and-fate approved these changes Apr 16, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 16, 2024

YangKeao requested a review from xhebox May 11, 2024 05:16

xhebox approved these changes May 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 May 11, 2024

ti-chi-bot bot merged commit 09c8f96 into pingcap:master May 11, 2024
23 checks passed

ti-chi-bot mentioned this pull request May 13, 2024

planner: fix the issue that UnionAll didn't handle the range bump case (#52542) #53209

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request May 13, 2024

This is an automated cherry-pick of pingcap#52542

3781969

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request May 13, 2024

planner: fix the issue that UnionAll didn't handle the range bump case (#52542) #53210

Closed

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request May 13, 2024

This is an automated cherry-pick of pingcap#52542

0fe415c

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request May 13, 2024

planner: fix the issue that UnionAll didn't handle the range bump case (#52542) #53211

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request May 13, 2024

This is an automated cherry-pick of pingcap#52542

ff905d1

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request May 13, 2024

planner: fix the issue that UnionAll didn't handle the range bump case (#52542) #53212

Open

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request May 13, 2024

planner: fix the issue that UnionAll didn't handle the range bump case (

15bbc92

#52542) (#53209) close #52472

terry1purcell pushed a commit to terry1purcell/tidb that referenced this pull request May 17, 2024

planner: fix the issue that UnionAll didn't handle the range bump case (

54ad5cd

pingcap#52542) close pingcap#52472

RidRisR pushed a commit to RidRisR/tidb that referenced this pull request May 23, 2024

planner: fix the issue that UnionAll didn't handle the range bump case (

674d76f

pingcap#52542) close pingcap#52472

ti-chi-bot bot pushed a commit that referenced this pull request May 23, 2024

planner: fix the issue that UnionAll didn't handle the range bump case (

8a21519

#52542) (#53211) close #52472

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: fix the issue that UnionAll didn't handle the range bump case #52542

planner: fix the issue that UnionAll didn't handle the range bump case #52542

YangKeao commented Apr 12, 2024 •

edited

Loading

codecov bot commented Apr 12, 2024 •

edited

Loading

YangKeao Apr 12, 2024

YangKeao Apr 15, 2024 •

edited

Loading

YangKeao commented Apr 15, 2024

time-and-fate left a comment

ti-chi-bot bot commented May 11, 2024

ti-chi-bot bot commented May 11, 2024

ti-chi-bot commented May 13, 2024

ti-chi-bot commented May 13, 2024

ti-chi-bot commented May 13, 2024

ti-chi-bot commented May 13, 2024

planner: fix the issue that UnionAll didn't handle the range bump case #52542

planner: fix the issue that UnionAll didn't handle the range bump case #52542

Conversation

YangKeao commented Apr 12, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

codecov bot commented Apr 12, 2024 • edited Loading

Codecov Report

YangKeao Apr 12, 2024

Choose a reason for hiding this comment

YangKeao Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

YangKeao commented Apr 15, 2024

time-and-fate left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented May 11, 2024

ti-chi-bot bot commented May 11, 2024

[LGTM Timeline notifier]

ti-chi-bot commented May 13, 2024

ti-chi-bot commented May 13, 2024

ti-chi-bot commented May 13, 2024

ti-chi-bot commented May 13, 2024

YangKeao commented Apr 12, 2024 •

edited

Loading

codecov bot commented Apr 12, 2024 •

edited

Loading

YangKeao Apr 15, 2024 •

edited

Loading