planner: unify OR type IndexMerge code paths #58396

time-and-fate · 2024-12-18T19:46:48Z

What problem does this PR solve?

Issue Number: ref #58361

What changed and how does it work?

Now generateORIndexMerge() in indexmerge_unfinished_path.go becomes the new entry for generating all OR type IndexMerge paths. All previous code paths are merged into this one.

Entry points

Previous entry in generateIndexMergeOrPaths() (the code path 1 mentioned in the issue) is modified, moved to indexmerge_unfinished_path.go, and becomes the new generateORIndexMerge().
Previous entries in generateIndexMergeOnDNF4MVIndex() and generateIndexMerge4ComposedIndex() (the code paths 2 and 3) are deleted and replaced by the new entry. If you look into the implementation, the code is almost the same as the new generateORIndexMerge().
Related function names and comments are also updated to reflect this change. You can check changes in generateIndexMergePath() for a simple overview.

Some details in unifying the code paths

As the old generateIndexMergeOrPaths() becomes the new generateORIndexMerge(), some logic in this function is deleted:
- The CanExprsPushDown() check partially becomes the existing same check in generateNormalIndexPartialPath(), partially becomes the newly added check in initUnfinishedPathsFromExpr().
- The "don't generate the IndexMerge path if all its partial paths use the same non-MV index" check is moved to buildIntoAccessPath()
- The calculation of AccessPath.CountAfterAccess is replaced by estimateCountAfterAccessForIndexMergeOR() which is introduced in the previous PR.
Checks for AccessPath.TableFilters, AccessPath.IndexFilters and the MaybeOverOptimized4PlanCache() check in matchPropForIndexMergeAlternatives() and generateNormalIndexPartialPath() are almost the same. They are unified and moved to buildIntoAccessPath().
For accessPathsForConds()
- previously there was a usage in code path 1 where the input candidatePaths is a slice. That is deleted now, so we can simplify accessPathsForConds() to only receive one *util.AccessPath and return one *util.AccessPath.
- Besides, the pruning logic for the empty/point ranges in it is moved to cmpAlternatives() now.
The needConsiderIndexMerge logic in generateIndexMerge4NormalIndex() (which becomes generateOtherIndexMerge() now) is modified.

Utils

In pkg/planner/util/misc.go, a util function SliceRecursiveFlattenIter() is added to iterate over multi-dimensional slices more elegantly. Otherwise, there will be some 5-level nested code blocks in this PR.

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

…-need-rewrite-or

…-need-rewrite-or-last

codecov · 2024-12-18T20:05:50Z

Codecov Report

Attention: Patch coverage is 92.10526% with 15 lines in your changes missing coverage. Please review.

Project coverage is 73.5860%. Comparing base (2a72e7f) to head (3bed336).
Report is 13 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #58396        +/-   ##
================================================
+ Coverage   73.5209%   73.5860%   +0.0650%     
================================================
  Files          1681       1680         -1     
  Lines        464398     466965      +2567     
================================================
+ Hits         341430     343621      +2191     
- Misses       102138     102455       +317     
- Partials      20830      20889        +59

Flag	Coverage Δ
integration	`43.0607% <92.1052%> (?)`
unit	`72.3003% <92.1052%> (+0.0324%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.7894% <ø> (+0.0029%)`	⬆️

…-need-rewrite-or-last

time-and-fate · 2024-12-25T13:01:23Z

/retest

pkg/planner/core/indexmerge_path.go

time-and-fate · 2024-12-25T13:14:11Z

pkg/planner/core/indexmerge_path.go

-	if !needConsiderIndexMerge {
-		return "IndexMerge is inapplicable or disabled. ", nil // IndexMerge is inapplicable
-	}


Since this limitation only applies to non-MV indexes, and now the MV index path is also generated here, as said in the design doc, we modify this limitation a little and move it to the end of this function.

It causes some extra unnecessary work now. This is also why there are new records in pkg/planner/cardinality/testdata/cardinality_suite_out.json and tests/integrationtest/r/imdbload.result.

time-and-fate · 2024-12-25T13:26:13Z

pkg/planner/core/indexmerge_path.go

-			continue
-		}
-		// in this loop we do two things.
-		// 1: If all the partialPaths use the same index, we will not use the indexMerge.


Moved to buildIntoAccessPath() in indexmerge_unfinished_path.go.

time-and-fate · 2024-12-25T13:26:17Z

pkg/planner/core/indexmerge_path.go

-		}
-		// in this loop we do two things.
-		// 1: If all the partialPaths use the same index, we will not use the indexMerge.
-		// 2: Compute a theoretical best countAfterAccess(pick its accessConds) for every alternative path(s).


Already moved into indexmerge_unfinished_path.go as estimateCountAfterAccessForIndexMergeOR() in the previous PR.

time-and-fate · 2024-12-25T13:32:03Z

pkg/planner/core/indexmerge_path.go

-	// identify whether all pushedDownCNFItems are fully used.
-	// If any partial path contains table filters, we need to keep the whole DNF filter in the Selection.
-	if len(partialPath.TableFilters) > 0 {
-		needSelection = true
-		partialPath.TableFilters = nil
-	}
-	// If any partial path's index filter cannot be pushed to TiKV, we should keep the whole DNF filter.
-	if len(partialPath.IndexFilters) != 0 && !expression.CanExprsPushDown(pushDownCtx, partialPath.IndexFilters, kv.TiKV) {
-		needSelection = true
-		// Clear IndexFilter, the whole filter will be put in indexMergePath.TableFilters.
-		partialPath.IndexFilters = nil
-	}
-	// Keep this filter as a part of table filters for safety if it has any parameter.
-	if expression.MaybeOverOptimized4PlanCache(ds.SCtx().GetExprCtx(), cnfItems) {
-		needSelection = true
-	}


Moved to buildIntoAccessPath() in indexmerge_unfinished_path.go.

time-and-fate · 2024-12-25T13:41:42Z

pkg/planner/core/indexmerge_path.go

-				if expression.CanExprsPushDown(pushDownCtx, []expression.Expression{cnfItem}, kv.TiKV) {
-					pushedDownCNFItems = append(pushedDownCNFItems, cnfItem)
-				} else {
-					shouldKeepCurrentFilter = true
-				}


In initUnfinishedPathsFromExpr():

For "case 1": It's already in generateNormalIndexPartialPath() so we don't need to add it again.

For "case 2" and "case 3": I added a similar check. Though I'm not sure if it's really needed, I added it anyway.

pkg/planner/core/find_best_task.go

time-and-fate · 2024-12-25T15:02:20Z

pkg/planner/core/indexmerge_unfinished_path.go

-				needSelection = len(remainingFilters) > 0 || len(unfinishedPath.idxColHasUsableFilter) > 0
+				needSelection = len(remainingFilters) > 0


Moved to the check at L211. If you look at it carefully, they are essentially the same.

Rustin170506

Thanks!

Rustin170506 · 2024-12-26T06:58:03Z

pkg/planner/core/indexmerge_path.go

-// generateIndexMerge4ComposedIndex generates index path composed of multi indexes including multivalued index from
-// (json_member_of / json_overlaps / json_contains) and single-valued index from normal indexes.
+// generateANDIndexMerge4ComposedIndex tries to generate AND type index merge AccessPath for (
+//json_member_of / json_overlaps / json_contains) on multiple multi-valued or normal indexes.


Is this formatted by the go fmt? I thought it would always keep a space here.

Updated.
This is formatted by GoLand's "wrap on typing", which is not very clever sometimes. It won't add a space in such places.
Actually, go fmt also won't add this space.

Rustin170506 · 2024-12-26T07:01:33Z

What changed and how does it work?

Maybe you fill it in as well for future archaeology.

tests/integrationtest/r/planner/core/casetest/physicalplantest/physical_plan.result

time-and-fate · 2024-12-26T09:13:28Z

What changed and how does it work?

Maybe you fill it in as well for future archaeology.

Updated.

AilinKid

Rest LGTM

pkg/planner/util/misc.go

AilinKid · 2024-12-26T09:55:13Z

pkg/planner/core/indexmerge_path.go

-					results = append(results, newPath)
-				} else {
-					results[0] = newPath
-					results = results[:1]


any notes for removing this？

Good catch.
My original idea is that the row count estimation logic should reflect the advantage of the empty range.
Anyway, I added this logic to the new implementation in the latest commit.
Probably it's better to keep things unchanged as much as possible in such a refactor.

AilinKid · 2024-12-26T10:01:46Z

pkg/planner/core/indexmerge_path.go

-			if finishedIndexMergePath != nil {
-				mvIndexPaths = append(mvIndexPaths, finishedIndexMergePath)
-			}
+		if !containMVPath {


seems tricky, but fine for now

tiprow · 2024-12-26T13:33:18Z

@time-and-fate: 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	`3bed336`	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-27T05:55:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, Rustin170506

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~~ [AilinKid,Rustin170506]
~~pkg/planner/OWNERS~~ [AilinKid,Rustin170506]

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-27T05:55:08Z

[LGTM Timeline notifier]

Timeline:

2024-12-26 07:00:15.417668994 +0000 UTC m=+1717805.506471536: ☑️ agreed by Rustin170506.
2024-12-27 05:55:07.463842697 +0000 UTC m=+70642.819847264: ☑️ agreed by AilinKid.

AilinKid · 2024-12-27T05:56:22Z

pkg/planner/core/indexmerge_unfinished_path.go

@@ -16,6 +16,7 @@ package core

 import (
 	"cmp"
+	"github.com/pingcap/tidb/pkg/sessionctx/variable"


group imports

time-and-fate added 26 commits December 10, 2024 01:54

refactor

3188f0b

Merge remote-tracking branch 'upstream/master' into s27-indexmerge-no…

4401a67

…-need-rewrite-or

fix lint

68cf5ef

update test

41e24b9

merge 2 mv index OR code paths into 1

e45d7a2

switch the last path to the new implementation

ac0378d

unify into single code path

a138caf

update test result

d72e454

Merge remote-tracking branch 'upstream/master' into s27-indexmerge-no…

d6fe22a

…-need-rewrite-or

fix fix control case and update test result

62b0180

fix missing index filters

f30186c

restore test result

37e4f39

update test result

e62949b

Merge remote-tracking branch 'upstream/master' into s27-indexmerge-no…

ce19969

…-need-rewrite-or

remove unneeded code

a431242

refactor and add comments 1

33b1cba

fix lint

63151a8

refactor comments

c38a0fe

Merge remote-tracking branch 'upstream/master' into s27-indexmerge-no…

7291678

…-need-rewrite-or

refactor

472b51c

refactor

c8bd4e5

refactor

b843a52

add comments

a2d0042

Merge remote-tracking branch 'upstream/master' into s27-indexmerge-no…

5cec70a

…-need-rewrite-or

add comments and small simplification

cb25d66

Merge remote-tracking branch 'upstream/master' into s27-indexmerge-no…

b80ab93

…-need-rewrite-or-last

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

time-and-fate added 4 commits December 25, 2024 17:24

minor improve sliceRecursiveFlattenIterHelper and add comments

dd3d870

Merge remote-tracking branch 'upstream/master' into s27-indexmerge-no…

34b8f95

…-need-rewrite-or-last

update BUILD.bazel and rename variable

31af6c6

Merge remote-tracking branch 'upstream/master' into s27-indexmerge-no…

4634e58

…-need-rewrite-or-last

time-and-fate changed the title ~~planner: [WIP]~~ planner: unify OR type IndexMerge code paths Dec 25, 2024

time-and-fate commented Dec 25, 2024

View reviewed changes

pkg/planner/core/indexmerge_path.go Show resolved Hide resolved

time-and-fate commented Dec 25, 2024

View reviewed changes

update

e18ead4

time-and-fate commented Dec 25, 2024

View reviewed changes

Rustin170506 approved these changes Dec 26, 2024

View reviewed changes

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

AilinKid reviewed Dec 26, 2024

View reviewed changes

tests/integrationtest/r/planner/core/casetest/physicalplantest/physical_plan.result Show resolved Hide resolved

format comments

9da5944

AilinKid reviewed Dec 26, 2024

View reviewed changes

time-and-fate added 2 commits December 26, 2024 21:17

add the previous pruning logic

39e4e7a

fix import order

3bed336

AilinKid approved these changes Dec 27, 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 27, 2024

AilinKid reviewed Dec 27, 2024

View reviewed changes

ti-chi-bot bot merged commit e44c60c into pingcap:master Dec 27, 2024
18 of 24 checks passed

time-and-fate mentioned this pull request Dec 27, 2024

Unify OR/Union type IndexMerge code paths to expand the applicable scenarios of existing capabilities #58361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: unify OR type IndexMerge code paths #58396

planner: unify OR type IndexMerge code paths #58396

time-and-fate commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

time-and-fate commented Dec 25, 2024

time-and-fate Dec 25, 2024

time-and-fate Dec 25, 2024 •

edited

Loading

time-and-fate Dec 25, 2024

time-and-fate Dec 25, 2024

time-and-fate Dec 25, 2024

time-and-fate Dec 25, 2024

time-and-fate Dec 25, 2024

Rustin170506 left a comment

Rustin170506 Dec 26, 2024

time-and-fate Dec 26, 2024 •

edited

Loading

Rustin170506 commented Dec 26, 2024

time-and-fate commented Dec 26, 2024

AilinKid left a comment

AilinKid Dec 26, 2024

time-and-fate Dec 26, 2024

AilinKid Dec 26, 2024

tiprow bot commented Dec 26, 2024

ti-chi-bot bot commented Dec 27, 2024

ti-chi-bot bot commented Dec 27, 2024

AilinKid Dec 27, 2024

		needSelection = len(remainingFilters) > 0 \|\| len(unfinishedPath.idxColHasUsableFilter) > 0
		needSelection = len(remainingFilters) > 0

planner: unify OR type IndexMerge code paths #58396

planner: unify OR type IndexMerge code paths #58396

Conversation

time-and-fate commented Dec 18, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Entry points

Some details in unifying the code paths

Utils

Check List

Release note

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

time-and-fate commented Dec 25, 2024

Choose a reason for hiding this comment

time-and-fate Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

time-and-fate Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Rustin170506 commented Dec 26, 2024

time-and-fate commented Dec 26, 2024

AilinKid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiprow bot commented Dec 26, 2024

ti-chi-bot bot commented Dec 27, 2024

ti-chi-bot bot commented Dec 27, 2024

[LGTM Timeline notifier]

Choose a reason for hiding this comment

time-and-fate commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

time-and-fate Dec 25, 2024 •

edited

Loading

time-and-fate Dec 26, 2024 •

edited

Loading