planner: add the functional dependency for Datasource, proj, select, agg #33071

winoros · 2022-03-15T06:58:53Z

What problem does this PR solve?

Issue Number: ref #29766

Problem Summary:

What is changed and how it works?

This pr adds the maintain of functional dependency for datasource, projection, selection, aggregation, inner join and semi join.

In next pr, we would add outer join and open the check.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

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

Documentation

Release note

None

ti-chi-bot · 2022-03-15T06:58:54Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

AilinKid
time-and-fate

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

planner/core/logical_plans.go

sre-bot · 2022-03-15T10:34:42Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/195153f832affa3f6810f468f4d56723c869de45

planner/core/stringer.go

planner/funcdep/fd_graph.go

expression/util.go

planner/core/logical_plans.go

time-and-fate · 2022-03-16T01:46:12Z

planner/funcdep/fd_graph.go

+// which can upgrade lax FDs to strict ones.
+func (s *FDSet) MakeNotNull(notNullCols FastIntSet) {
+	notNullCols.UnionWith(s.NotNullCols)
+	notNullColsSet := s.closureOfEquivalence(notNullCols)


Do we need to exclude lax equivalence in it?

This time we only maintain the strict equiv classes.

time-and-fate · 2022-03-16T01:59:50Z

planner/funcdep/fd_graph.go

+			if fd.from.SubsetOf(notNullColsSet) && fd.to.SubsetOf(notNullColsSet) {
+				// we don't need to clean the old lax FD because when adding the corresponding strict one, the lax
+				// one will be implied by that and itself is removed.
+				s.AddStrictFunctionalDependency(fd.from, fd.to)


Seems it's possible that it's an equivalence here.

And, since we don't maintain the lax equiv classes, we can make sure this fd must be fd instead of equiv class when we go here

time-and-fate · 2022-03-16T04:49:41Z

planner/core/logical_plans.go

+	} else {
+		for k, v := range rightFD.HashCodeToUniqueID {
+			if _, ok := fds.HashCodeToUniqueID[k]; ok {
+				panic("shouldn't be here, children has same expr while registered not only once")


Please avoid panic.

time-and-fate · 2022-03-16T05:36:52Z

planner/core/logical_plans.go

+				ok               bool
+				constantUniqueID int
+			)
+			if constantUniqueID, ok = fds.IsHashCodeRegistered(hashCode); !ok {


What would happen for the ok case?
If the same constant appears in different operators but corresponds to different UniqueID, will there be mistakes?

The same constant will use the same hashcode, then use the same unique id here.

This is temp solution to fix it before we refactor the way we store the const.

time-and-fate · 2022-03-16T05:51:49Z

planner/core/logical_plans.go

+			result := expression.EvaluateExprWithNull(p.ctx, p.schema, x)
+			con, ok := result.(*expression.Constant)
+			if !ok || con.Value.IsNull() {
+				// if x can be nullable when referred columns are null, the extended column can be nullable.
+				nullable = true
+			}
+			if !nullable || determinants.SubsetOf(fds.NotNullCols) {


I think it's the same as isNullRejected.

time-and-fate · 2022-03-16T05:53:12Z

planner/core/logical_plans.go

+			case *expression.ScalarFunction:
+				scalarUniqueID, ok := fds.IsHashCodeRegistered(string(hack.String(x.HashCode(p.SCtx().GetSessionVars().StmtCtx))))
+				if !ok {
+					panic("selected expr must have been registered, shouldn't be here")


Please avoid panic.

planner/core/logical_plan_builder.go

time-and-fate · 2022-03-16T07:05:01Z

planner/core/logical_plan_builder.go

+				// 1: normal value can be multiple
+				// 2: null value can be multiple
+				// for this kind of lax to be strict, we need to make both the determinant and dependency not-null.
+				fds.AddLaxFunctionalDependency(keyCols, allCols, false)


Could non-unique index become functional dependency?

No
a b
1 2
1 3
2 2

There's an index on (a).
You can see that we cannot build either strict or lax functional dependency.

make sense, we should remove this

time-and-fate · 2022-03-16T08:38:35Z

planner/core/logical_plans.go

+	fds.NotNullCols.UnionWith(rightFD.NotNullCols)
+	if fds.HashCodeToUniqueID == nil {
+		fds.HashCodeToUniqueID = rightFD.HashCodeToUniqueID
+	} else {
+		for k, v := range rightFD.HashCodeToUniqueID {
+			if _, ok := fds.HashCodeToUniqueID[k]; ok {
+				panic("shouldn't be here, children has same expr while registered not only once")
+			}
+			fds.HashCodeToUniqueID[k] = v
+		}
+	}
+	for i, ok := rightFD.GroupByCols.Next(0); ok; i, ok = rightFD.GroupByCols.Next(i + 1) {
+		fds.GroupByCols.Insert(i)
+	}
+	fds.HasAggBuilt = fds.HasAggBuilt || rightFD.HasAggBuilt


I think they should be put into MakeCartesianProduct.

MakeCartesianProduct will be also used in outer join. So we cannot put it into it.

time-and-fate · 2022-03-16T08:59:01Z

planner/core/logical_plans.go

+	eqCondSlice := expression.ScalarFuncs2Exprs(p.EqualConditions)
+	// some join eq conditions are stored in the OtherConditions.
+	allConds := append(eqCondSlice, p.OtherConditions...)


Do we need to consider LeftConditions and RightConditions?

We only need to consider that in outer join.

winoros · 2022-03-17T02:27:29Z

/merge

ti-chi-bot · 2022-03-17T02:27:32Z

This pull request has been accepted and is ready to merge.

Commit hash: faa629c

ti-chi-bot · 2022-03-17T03:36:41Z

@winoros: PR needs rebase.

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/test-infra repository.

… fd-not-outer-join-and-open

winoros · 2022-03-17T08:45:29Z

/merge

ti-chi-bot · 2022-03-17T08:45:32Z

This pull request has been accepted and is ready to merge.

Commit hash: 9c1d963

winoros · 2022-03-17T09:06:33Z

@ti-chi-bot

winoros · 2022-03-17T09:10:07Z

@ti-chi-bot

winoros · 2022-03-17T09:14:17Z

/merge

ti-chi-bot · 2022-03-17T11:48:34Z

@winoros: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

winoros added 3 commits March 15, 2022 14:16

planner: add the functional dependency for Datasource, proj, select, agg

ed4e051

fix lint

748ba62

undo some changes

6b2043f

winoros requested a review from a team as a code owner March 15, 2022 06:58

ti-chi-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. labels Mar 15, 2022

AilinKid reviewed Mar 15, 2022

View reviewed changes

planner/core/logical_plans.go Outdated Show resolved Hide resolved

winoros force-pushed the fd-not-outer-join-and-open branch from 404db64 to 7433d71 Compare March 15, 2022 08:21

fix tests

ba60938

winoros force-pushed the fd-not-outer-join-and-open branch from 7433d71 to ba60938 Compare March 15, 2022 08:57

to #12

c06fc01

add license header and fix check and fix import

39f3c78

time-and-fate reviewed Mar 15, 2022

View reviewed changes

planner/core/stringer.go Outdated Show resolved Hide resolved

planner/funcdep/fd_graph.go Outdated Show resolved Hide resolved

planner/funcdep/fd_graph.go Outdated Show resolved Hide resolved

expression/util.go Show resolved Hide resolved

qw4990 reviewed Mar 15, 2022

View reviewed changes

planner/core/logical_plans.go Show resolved Hide resolved

address comments

b357c0e

winoros force-pushed the fd-not-outer-join-and-open branch from cf33ad7 to b357c0e Compare March 15, 2022 14:07