From b15b9baca14298748533d626cad8ed024fa7521b Mon Sep 17 00:00:00 2001 From: Dousir9 <736191200@qq.com> Date: Sun, 15 Jan 2023 00:39:04 +0800 Subject: [PATCH 01/10] the proj below sort will prevent optimizer from pushing the proj down to TiDB/TiFlash --- planner/core/rule_topn_push_down.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/planner/core/rule_topn_push_down.go b/planner/core/rule_topn_push_down.go index 5dacac7579caa..038e56392463c 100644 --- a/planner/core/rule_topn_push_down.go +++ b/planner/core/rule_topn_push_down.go @@ -82,6 +82,13 @@ func (ls *LogicalSort) pushDownTopN(topN *LogicalTopN, opt *logicalOptimizeOp) L return ls.children[0].pushDownTopN(topN, opt) } // If a TopN is pushed down, this sort is useless. + // The proj below sort will prevent optimizer from pushing the proj down to TiDB/TiFlash. + if projection, ok := ls.children[0].(*LogicalProjection); ok { + for i, child := range projection.Children() { + projection.Children()[i] = child.pushDownTopN(nil, opt) + } + return topN.setChild(projection, opt) + } return ls.children[0].pushDownTopN(topN, opt) } From 7bca49442087935f5abefb4bc18ddf14ba301ac4 Mon Sep 17 00:00:00 2001 From: Dousir9 <736191200@qq.com> Date: Sun, 15 Jan 2023 00:42:42 +0800 Subject: [PATCH 02/10] add unit test --- planner/core/rule_topn_push_down_test.go | 35 ++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 planner/core/rule_topn_push_down_test.go diff --git a/planner/core/rule_topn_push_down_test.go b/planner/core/rule_topn_push_down_test.go new file mode 100644 index 0000000000000..cd8583c40d94f --- /dev/null +++ b/planner/core/rule_topn_push_down_test.go @@ -0,0 +1,35 @@ +// Copyright 2023 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package core_test + +import ( + "testing" + + "github.com/pingcap/tidb/kv" + "github.com/pingcap/tidb/testkit" + "github.com/stretchr/testify/require" +) + +func TestIssue40535(t *testing.T) { + store := testkit.CreateMockStore(t) + var cfg kv.InjectionConfig + tk := testkit.NewTestKit(t, kv.NewInjectedStore(store, &cfg)) + tk.MustExec("use test;") + tk.MustExec("drop table if exists t1; drop table if exists t2;") + tk.MustExec("CREATE TABLE `t1`(`c1` bigint(20) NOT NULL DEFAULT '-2312745469307452950', `c2` datetime DEFAULT '5316-02-03 06:54:49', `c3` tinyblob DEFAULT NULL, PRIMARY KEY (`c1`) /*T![clustered_index] CLUSTERED */) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin;") + tk.MustExec("CREATE TABLE `t2`(`c1` set('kn8pu','7et','vekx6','v3','liwrh','q14','1met','nnd5i','5o0','8cz','l') DEFAULT '7et,vekx6,liwrh,q14,1met', `c2` float DEFAULT '1.683167', KEY `k1` (`c2`,`c1`), KEY `k2` (`c2`)) ENGINE=InnoDB DEFAULT CHARSET=gbk COLLATE=gbk_chinese_ci;") + tk.MustExec("(select /*+ agg_to_cop()*/ locate(t1.c3, t1.c3) as r0, t1.c3 as r1 from t1 where not( IsNull(t1.c1)) order by r0,r1) union all (select concat_ws(',', t2.c2, t2.c1) as r0, t2.c1 as r1 from t2 order by r0, r1) order by 1 limit 273;") + require.Empty(t, tk.Session().LastMessage()) +} From 0c43e9a8df2951633c566a962560c77be583ccbb Mon Sep 17 00:00:00 2001 From: Dousir9 <736191200@qq.com> Date: Sun, 15 Jan 2023 00:56:04 +0800 Subject: [PATCH 03/10] fix-typo --- planner/core/rule_topn_push_down.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planner/core/rule_topn_push_down.go b/planner/core/rule_topn_push_down.go index 038e56392463c..7040ea6a20ae4 100644 --- a/planner/core/rule_topn_push_down.go +++ b/planner/core/rule_topn_push_down.go @@ -82,7 +82,7 @@ func (ls *LogicalSort) pushDownTopN(topN *LogicalTopN, opt *logicalOptimizeOp) L return ls.children[0].pushDownTopN(topN, opt) } // If a TopN is pushed down, this sort is useless. - // The proj below sort will prevent optimizer from pushing the proj down to TiDB/TiFlash. + // The proj below sort will prevent optimizer from pushing the topN down to TiDB/TiFlash. if projection, ok := ls.children[0].(*LogicalProjection); ok { for i, child := range projection.Children() { projection.Children()[i] = child.pushDownTopN(nil, opt) From 7e9ba52f0f3cde6098b7a3a0a59e2637d624fafa Mon Sep 17 00:00:00 2001 From: Dousir9 <736191200@qq.com> Date: Mon, 16 Jan 2023 23:58:11 +0800 Subject: [PATCH 04/10] put the test case into the existing file. --- planner/core/plan_test.go | 13 +++++++++ planner/core/rule_topn_push_down_test.go | 35 ------------------------ 2 files changed, 13 insertions(+), 35 deletions(-) delete mode 100644 planner/core/rule_topn_push_down_test.go diff --git a/planner/core/plan_test.go b/planner/core/plan_test.go index f1c65fcd9fb4d..a449cee57e69f 100644 --- a/planner/core/plan_test.go +++ b/planner/core/plan_test.go @@ -25,6 +25,7 @@ import ( "github.com/pingcap/tidb/config" "github.com/pingcap/tidb/expression" "github.com/pingcap/tidb/expression/aggregation" + "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/parser/ast" "github.com/pingcap/tidb/parser/model" "github.com/pingcap/tidb/parser/mysql" @@ -1142,3 +1143,15 @@ func TestJSONPlanInExplain(t *testing.T) { } } } + +func TestIssue40535(t *testing.T) { + store := testkit.CreateMockStore(t) + var cfg kv.InjectionConfig + tk := testkit.NewTestKit(t, kv.NewInjectedStore(store, &cfg)) + tk.MustExec("use test;") + tk.MustExec("drop table if exists t1; drop table if exists t2;") + tk.MustExec("CREATE TABLE `t1`(`c1` bigint(20) NOT NULL DEFAULT '-2312745469307452950', `c2` datetime DEFAULT '5316-02-03 06:54:49', `c3` tinyblob DEFAULT NULL, PRIMARY KEY (`c1`) /*T![clustered_index] CLUSTERED */) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin;") + tk.MustExec("CREATE TABLE `t2`(`c1` set('kn8pu','7et','vekx6','v3','liwrh','q14','1met','nnd5i','5o0','8cz','l') DEFAULT '7et,vekx6,liwrh,q14,1met', `c2` float DEFAULT '1.683167', KEY `k1` (`c2`,`c1`), KEY `k2` (`c2`)) ENGINE=InnoDB DEFAULT CHARSET=gbk COLLATE=gbk_chinese_ci;") + tk.MustExec("(select /*+ agg_to_cop()*/ locate(t1.c3, t1.c3) as r0, t1.c3 as r1 from t1 where not( IsNull(t1.c1)) order by r0,r1) union all (select concat_ws(',', t2.c2, t2.c1) as r0, t2.c1 as r1 from t2 order by r0, r1) order by 1 limit 273;") + require.Empty(t, tk.Session().LastMessage()) +} diff --git a/planner/core/rule_topn_push_down_test.go b/planner/core/rule_topn_push_down_test.go deleted file mode 100644 index cd8583c40d94f..0000000000000 --- a/planner/core/rule_topn_push_down_test.go +++ /dev/null @@ -1,35 +0,0 @@ -// Copyright 2023 PingCAP, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package core_test - -import ( - "testing" - - "github.com/pingcap/tidb/kv" - "github.com/pingcap/tidb/testkit" - "github.com/stretchr/testify/require" -) - -func TestIssue40535(t *testing.T) { - store := testkit.CreateMockStore(t) - var cfg kv.InjectionConfig - tk := testkit.NewTestKit(t, kv.NewInjectedStore(store, &cfg)) - tk.MustExec("use test;") - tk.MustExec("drop table if exists t1; drop table if exists t2;") - tk.MustExec("CREATE TABLE `t1`(`c1` bigint(20) NOT NULL DEFAULT '-2312745469307452950', `c2` datetime DEFAULT '5316-02-03 06:54:49', `c3` tinyblob DEFAULT NULL, PRIMARY KEY (`c1`) /*T![clustered_index] CLUSTERED */) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin;") - tk.MustExec("CREATE TABLE `t2`(`c1` set('kn8pu','7et','vekx6','v3','liwrh','q14','1met','nnd5i','5o0','8cz','l') DEFAULT '7et,vekx6,liwrh,q14,1met', `c2` float DEFAULT '1.683167', KEY `k1` (`c2`,`c1`), KEY `k2` (`c2`)) ENGINE=InnoDB DEFAULT CHARSET=gbk COLLATE=gbk_chinese_ci;") - tk.MustExec("(select /*+ agg_to_cop()*/ locate(t1.c3, t1.c3) as r0, t1.c3 as r1 from t1 where not( IsNull(t1.c1)) order by r0,r1) union all (select concat_ws(',', t2.c2, t2.c1) as r0, t2.c1 as r1 from t2 order by r0, r1) order by 1 limit 273;") - require.Empty(t, tk.Session().LastMessage()) -} From a87d5ab969e4d45fe2fa5c87152ff7f6850f4d0d Mon Sep 17 00:00:00 2001 From: Dousir9 <736191200@qq.com> Date: Wed, 18 Jan 2023 00:16:14 +0800 Subject: [PATCH 05/10] check whether proj can be eliminated or contains a column in topN.ByItems --- planner/core/rule_topn_push_down.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/planner/core/rule_topn_push_down.go b/planner/core/rule_topn_push_down.go index 7040ea6a20ae4..73f63f83894f3 100644 --- a/planner/core/rule_topn_push_down.go +++ b/planner/core/rule_topn_push_down.go @@ -82,12 +82,20 @@ func (ls *LogicalSort) pushDownTopN(topN *LogicalTopN, opt *logicalOptimizeOp) L return ls.children[0].pushDownTopN(topN, opt) } // If a TopN is pushed down, this sort is useless. - // The proj below sort will prevent optimizer from pushing the topN down to TiDB/TiFlash. - if projection, ok := ls.children[0].(*LogicalProjection); ok { - for i, child := range projection.Children() { - projection.Children()[i] = child.pushDownTopN(nil, opt) + if projection, ok := ls.children[0].(*LogicalProjection); ok && !canProjectionBeEliminatedLoose(projection) { + // If the proj below sort cannot be eliminated and contains a column in topN.ByItems, + // proj will prevent the optimizer from pushing topN down. + for _, by := range topN.ByItems { + cols := expression.ExtractColumns(by.Expr) + for _, col := range cols { + if projection.Schema().Contains(col) { + for i, child := range projection.Children() { + projection.Children()[i] = child.pushDownTopN(nil, opt) + } + return topN.setChild(projection, opt) + } + } } - return topN.setChild(projection, opt) } return ls.children[0].pushDownTopN(topN, opt) } From 5edf750148b1fdfda849e0f9a111e32c54e98b7b Mon Sep 17 00:00:00 2001 From: Dousir9 <736191200@qq.com> Date: Wed, 18 Jan 2023 01:08:52 +0800 Subject: [PATCH 06/10] remove unnecessary check --- planner/core/rule_topn_push_down.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/planner/core/rule_topn_push_down.go b/planner/core/rule_topn_push_down.go index 73f63f83894f3..34217da73165a 100644 --- a/planner/core/rule_topn_push_down.go +++ b/planner/core/rule_topn_push_down.go @@ -82,9 +82,8 @@ func (ls *LogicalSort) pushDownTopN(topN *LogicalTopN, opt *logicalOptimizeOp) L return ls.children[0].pushDownTopN(topN, opt) } // If a TopN is pushed down, this sort is useless. - if projection, ok := ls.children[0].(*LogicalProjection); ok && !canProjectionBeEliminatedLoose(projection) { - // If the proj below sort cannot be eliminated and contains a column in topN.ByItems, - // proj will prevent the optimizer from pushing topN down. + if projection, ok := ls.children[0].(*LogicalProjection); ok { + // If the proj below sort contains a column in topN.ByItems, proj will prevent the optimizer from pushing topN down. for _, by := range topN.ByItems { cols := expression.ExtractColumns(by.Expr) for _, col := range cols { From 3114b6b19bcf956017b68c4c50fc4d0d30a84276 Mon Sep 17 00:00:00 2001 From: Dousir9 <736191200@qq.com> Date: Thu, 19 Jan 2023 20:01:43 +0800 Subject: [PATCH 07/10] change the code of LogicalProjection.pushDownTopN instead of the LogicalSort.pushDownTopN --- planner/core/rule_topn_push_down.go | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/planner/core/rule_topn_push_down.go b/planner/core/rule_topn_push_down.go index 34217da73165a..56efcdf71af9b 100644 --- a/planner/core/rule_topn_push_down.go +++ b/planner/core/rule_topn_push_down.go @@ -82,20 +82,6 @@ func (ls *LogicalSort) pushDownTopN(topN *LogicalTopN, opt *logicalOptimizeOp) L return ls.children[0].pushDownTopN(topN, opt) } // If a TopN is pushed down, this sort is useless. - if projection, ok := ls.children[0].(*LogicalProjection); ok { - // If the proj below sort contains a column in topN.ByItems, proj will prevent the optimizer from pushing topN down. - for _, by := range topN.ByItems { - cols := expression.ExtractColumns(by.Expr) - for _, col := range cols { - if projection.Schema().Contains(col) { - for i, child := range projection.Children() { - projection.Children()[i] = child.pushDownTopN(nil, opt) - } - return topN.setChild(projection, opt) - } - } - } - } return ls.children[0].pushDownTopN(topN, opt) } @@ -150,6 +136,17 @@ func (p *LogicalProjection) pushDownTopN(topN *LogicalTopN, opt *logicalOptimize topN.ByItems = append(topN.ByItems[:i], topN.ByItems[i+1:]...) } } + + // if the projection contains a column in topN.ByItems, projection will prevent the optimizer from pushing topN down. + for _, by := range topN.ByItems { + cols := expression.ExtractColumns(by.Expr) + for _, col := range cols { + if p.Schema().Contains(col) { + p.children[0] = p.children[0].pushDownTopN(nil, opt) + return topN.setChild(p, opt) + } + } + } } p.children[0] = p.children[0].pushDownTopN(topN, opt) return p From 07d52255f9abfd667003891206d33a555ff20ae6 Mon Sep 17 00:00:00 2001 From: Dousir9 <736191200@qq.com> Date: Mon, 30 Jan 2023 16:47:51 +0800 Subject: [PATCH 08/10] fix: with Column.ID = 0 --- planner/core/rule_topn_push_down.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/planner/core/rule_topn_push_down.go b/planner/core/rule_topn_push_down.go index 56efcdf71af9b..ec9b44f3fd5ee 100644 --- a/planner/core/rule_topn_push_down.go +++ b/planner/core/rule_topn_push_down.go @@ -137,11 +137,11 @@ func (p *LogicalProjection) pushDownTopN(topN *LogicalTopN, opt *logicalOptimize } } - // if the projection contains a column in topN.ByItems, projection will prevent the optimizer from pushing topN down. + // if the projection contains a column(with ID=0) in topN.ByItems, projection will prevent the optimizer from pushing topN down. for _, by := range topN.ByItems { cols := expression.ExtractColumns(by.Expr) for _, col := range cols { - if p.Schema().Contains(col) { + if p.Schema().Contains(col) && col.ID == 0 { p.children[0] = p.children[0].pushDownTopN(nil, opt) return topN.setChild(p, opt) } From dd9ac2d18ac8f3bd752e1bd555c0661296224b13 Mon Sep 17 00:00:00 2001 From: Dousir9 <736191200@qq.com> Date: Mon, 30 Jan 2023 17:57:14 +0800 Subject: [PATCH 09/10] check whether the column is generated by projection --- planner/core/rule_topn_push_down.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/planner/core/rule_topn_push_down.go b/planner/core/rule_topn_push_down.go index ec9b44f3fd5ee..e4bcd7899bae8 100644 --- a/planner/core/rule_topn_push_down.go +++ b/planner/core/rule_topn_push_down.go @@ -141,9 +141,12 @@ func (p *LogicalProjection) pushDownTopN(topN *LogicalTopN, opt *logicalOptimize for _, by := range topN.ByItems { cols := expression.ExtractColumns(by.Expr) for _, col := range cols { - if p.Schema().Contains(col) && col.ID == 0 { - p.children[0] = p.children[0].pushDownTopN(nil, opt) - return topN.setChild(p, opt) + if col.ID == 0 && p.Schema().Contains(col) { + if !p.children[0].Schema().Contains(col) { + // the column is generated by projection + p.children[0] = p.children[0].pushDownTopN(nil, opt) + return topN.setChild(p, opt) + } } } } From 1a3ce1ba95cd0f00f4a5124d242290e19abbd5fb Mon Sep 17 00:00:00 2001 From: Dousir9 <736191200@qq.com> Date: Mon, 30 Jan 2023 19:02:58 +0800 Subject: [PATCH 10/10] update comment --- planner/core/rule_topn_push_down.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/planner/core/rule_topn_push_down.go b/planner/core/rule_topn_push_down.go index e4bcd7899bae8..2b4998049911a 100644 --- a/planner/core/rule_topn_push_down.go +++ b/planner/core/rule_topn_push_down.go @@ -137,13 +137,13 @@ func (p *LogicalProjection) pushDownTopN(topN *LogicalTopN, opt *logicalOptimize } } - // if the projection contains a column(with ID=0) in topN.ByItems, projection will prevent the optimizer from pushing topN down. + // if topN.ByItems contains a column(with ID=0) generated by projection, projection will prevent the optimizer from pushing topN down. for _, by := range topN.ByItems { cols := expression.ExtractColumns(by.Expr) for _, col := range cols { if col.ID == 0 && p.Schema().Contains(col) { + // check whether the column is generated by projection if !p.children[0].Schema().Contains(col) { - // the column is generated by projection p.children[0] = p.children[0].pushDownTopN(nil, opt) return topN.setChild(p, opt) }