From 05d4ace64c055de970b4e3a78b35a072d9b08c1f Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 20 Oct 2023 17:35:05 +0800 Subject: [PATCH 1/6] planner: add test for issue 47331 Signed-off-by: hi-rustin --- executor/explain_test.go | 51 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/executor/explain_test.go b/executor/explain_test.go index 448cca95428a2..d1342fdd6e25a 100644 --- a/executor/explain_test.go +++ b/executor/explain_test.go @@ -188,6 +188,57 @@ func checkMemoryInfo(t *testing.T, tk *testkit.TestKit, sql string) { } } +func TestIssue47331(t *testing.T) { + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + tk.MustExec("drop table if exists t1") + tk.MustExec(`create table t1( + id1 varchar(2) DEFAULT '00', + id2 varchar(30) NOT NULL, + id3 datetime DEFAULT NULL, + id4 varchar(100) NOT NULL DEFAULT 'ecifdata', + id5 datetime NOT NULL DEFAULT CURRENT_TIMESTAMP, + id6 int(11) DEFAULT NULL, + id7 int(11) DEFAULT NULL, + UNIQUE KEY UI_id2 (id2), + KEY ix_id1 (id1) + )`) + tk.MustExec("drop table if exists t2") + tk.MustExec(`create table t2( + id10 varchar(40) NOT NULL, + id2 varchar(30) NOT NULL, + KEY IX_id2 (id2), + PRIMARY KEY (id10) + )`) + tk.MustExec("drop table if exists t3") + tk.MustExec(`create table t3( + id20 varchar(40) DEFAULT NULL, + UNIQUE KEY IX_id20 (id20) + )`) + tk.MustExec(` + explain + UPDATE t1 a + SET a.id1 = '04', + a.id3 = CURRENT_TIMESTAMP, + a.id4 = SUBSTRING_INDEX(USER(), '@', 1), + a.id5 = CURRENT_TIMESTAMP + WHERE a.id1 = '03' + AND a.id6 - IFNULL(a.id7, 0) = + ( + SELECT COUNT(1) + FROM t2 b, t3 c + WHERE b.id10 = c.id20 + AND b.id2 = a.id2 + AND b.id2 in ( + SELECT rn.id2 + FROM t1 rn + WHERE rn.id1 = '03' + ) + ); + `) +} + func TestMemoryAndDiskUsageAfterClose(t *testing.T) { store := testkit.CreateMockStore(t) tk := testkit.NewTestKit(t, store) From 2f3ec60c5beaa99f4629d5ae2de05796465c6c4c Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 22 Nov 2023 12:14:11 +0800 Subject: [PATCH 2/6] Clone the originalSchema Signed-off-by: hi-rustin --- planner/core/rule_join_reorder.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/planner/core/rule_join_reorder.go b/planner/core/rule_join_reorder.go index c5a721f4659d0..0c3d5b7e36763 100644 --- a/planner/core/rule_join_reorder.go +++ b/planner/core/rule_join_reorder.go @@ -255,7 +255,8 @@ func (s *joinReOrderSolver) optimizeRecursive(ctx sessionctx.Context, p LogicalP proj := LogicalProjection{ Exprs: expression.Column2Exprs(originalSchema.Columns), }.Init(p.SCtx(), p.SelectBlockOffset()) - proj.SetSchema(originalSchema) + // Clone the schema here, because the schema may be changed by the projection optimization later. + proj.SetSchema(originalSchema.Clone()) proj.SetChildren(p) p = proj } From 6fb5f70aa9b2e170691ecadeba2ba106cb1995ab Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 22 Nov 2023 14:36:12 +0800 Subject: [PATCH 3/6] Better comment Signed-off-by: hi-rustin --- planner/core/rule_join_reorder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planner/core/rule_join_reorder.go b/planner/core/rule_join_reorder.go index 0c3d5b7e36763..5465ac6675da8 100644 --- a/planner/core/rule_join_reorder.go +++ b/planner/core/rule_join_reorder.go @@ -255,7 +255,7 @@ func (s *joinReOrderSolver) optimizeRecursive(ctx sessionctx.Context, p LogicalP proj := LogicalProjection{ Exprs: expression.Column2Exprs(originalSchema.Columns), }.Init(p.SCtx(), p.SelectBlockOffset()) - // Clone the schema here, because the schema may be changed by the projection optimization later. + // Clone the schema here, because the schema may be changed by column pruning rules. proj.SetSchema(originalSchema.Clone()) proj.SetChildren(p) p = proj From a9cdd4701aa9c85aca1f0d10dd4bbc05b5ebcd3e Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sun, 26 Nov 2023 11:26:49 +0800 Subject: [PATCH 4/6] Add new line Signed-off-by: hi-rustin --- executor/explain_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/executor/explain_test.go b/executor/explain_test.go index d1342fdd6e25a..90184d948a9be 100644 --- a/executor/explain_test.go +++ b/executor/explain_test.go @@ -369,6 +369,7 @@ func TestCheckActRowsWithUnistore(t *testing.T) { config.UpdateGlobal(func(conf *config.Config) { conf.EnableCollectExecutionInfo = true }) + store := testkit.CreateMockStore(t) // testSuite1 use default mockstore which is unistore tk := testkit.NewTestKit(t, store) From 8f5a96f7c4977840d263aa2b897941aaf5786804 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sun, 26 Nov 2023 12:36:00 +0800 Subject: [PATCH 5/6] Sleep a while Signed-off-by: hi-rustin --- executor/explain_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/executor/explain_test.go b/executor/explain_test.go index 90184d948a9be..9888df6babaf4 100644 --- a/executor/explain_test.go +++ b/executor/explain_test.go @@ -381,6 +381,7 @@ func TestCheckActRowsWithUnistore(t *testing.T) { tk.MustExec("analyze table t_unistore_act_rows") tk.MustExec("set @@tidb_merge_join_concurrency= 5;") + time.Sleep(time.Second * 5) type testStruct struct { sql string expected []string From 9be51ea36c39b2acab2457d8b2ec2ed3e9bc9abd Mon Sep 17 00:00:00 2001 From: Weizhen Wang Date: Sun, 26 Nov 2023 21:21:24 +0800 Subject: [PATCH 6/6] update Signed-off-by: Weizhen Wang --- executor/explain_test.go | 93 -------------------- executor/test/explain/BUILD.bazel | 15 ++++ executor/test/explain/explain_test.go | 117 ++++++++++++++++++++++++++ executor/test/explain/main_test.go | 30 +++++++ 4 files changed, 162 insertions(+), 93 deletions(-) create mode 100644 executor/test/explain/BUILD.bazel create mode 100644 executor/test/explain/explain_test.go create mode 100644 executor/test/explain/main_test.go diff --git a/executor/explain_test.go b/executor/explain_test.go index 9888df6babaf4..fa5ed5a055f6c 100644 --- a/executor/explain_test.go +++ b/executor/explain_test.go @@ -24,7 +24,6 @@ import ( "testing" "time" - "github.com/pingcap/tidb/config" "github.com/pingcap/tidb/errno" "github.com/pingcap/tidb/parser/auth" plannercore "github.com/pingcap/tidb/planner/core" @@ -350,98 +349,6 @@ func checkActRowsNotEmpty(t *testing.T, tk *testkit.TestKit, sql string) { } } -func checkActRows(t *testing.T, tk *testkit.TestKit, sql string, expected []string) { - actRowsCol := 2 - rows := tk.MustQuery("explain analyze " + sql).Rows() - require.Equal(t, len(expected), len(rows)) - for id, row := range rows { - strs := make([]string, len(row)) - for i, c := range row { - strs[i] = c.(string) - } - - require.Equal(t, expected[id], strs[actRowsCol], fmt.Sprintf("error comparing %s", sql)) - } -} - -func TestCheckActRowsWithUnistore(t *testing.T) { - defer config.RestoreFunc()() - config.UpdateGlobal(func(conf *config.Config) { - conf.EnableCollectExecutionInfo = true - }) - - store := testkit.CreateMockStore(t) - // testSuite1 use default mockstore which is unistore - tk := testkit.NewTestKit(t, store) - tk.MustExec("use test") - tk.MustExec("set tidb_cost_model_version=2") - tk.MustExec("drop table if exists t_unistore_act_rows") - tk.MustExec("create table t_unistore_act_rows(a int, b int, index(a, b))") - tk.MustExec("insert into t_unistore_act_rows values (1, 0), (1, 0), (2, 0), (2, 1)") - tk.MustExec("analyze table t_unistore_act_rows") - tk.MustExec("set @@tidb_merge_join_concurrency= 5;") - - time.Sleep(time.Second * 5) - type testStruct struct { - sql string - expected []string - } - - tests := []testStruct{ - { - sql: "select * from t_unistore_act_rows", - expected: []string{"4", "4"}, - }, - { - sql: "select * from t_unistore_act_rows where a > 1", - expected: []string{"2", "2"}, - }, - { - sql: "select * from t_unistore_act_rows where a > 1 and b > 0", - expected: []string{"1", "1", "2"}, - }, - { - sql: "select b from t_unistore_act_rows", - expected: []string{"4", "4"}, - }, - { - sql: "select * from t_unistore_act_rows where b > 0", - expected: []string{"1", "1", "4"}, - }, - { - sql: "select count(*) from t_unistore_act_rows", - expected: []string{"1", "1", "1", "4"}, - }, - { - sql: "select count(*) from t_unistore_act_rows group by a", - expected: []string{"2", "2", "2", "4"}, - }, - { - sql: "select count(*) from t_unistore_act_rows group by b", - expected: []string{"2", "4", "4"}, - }, - { - sql: "with cte(a) as (select a from t_unistore_act_rows) select (select 1 from cte limit 1) from cte;", - expected: []string{"4", "1", "1", "1", "4", "4", "4", "4", "4"}, - }, - { - sql: "select a, row_number() over (partition by b) from t_unistore_act_rows;", - expected: []string{"4", "4", "4", "4", "4", "4", "4"}, - }, - { - sql: "select /*+ merge_join(t1, t2) */ * from t_unistore_act_rows t1 join t_unistore_act_rows t2 on t1.b = t2.b;", - expected: []string{"10", "10", "4", "4", "4", "4", "4", "4", "4", "4", "4", "4"}, - }, - } - - // Default RPC encoding may cause statistics explain result differ and then the test unstable. - tk.MustExec("set @@tidb_enable_chunk_rpc = on") - - for _, test := range tests { - checkActRows(t, tk, test.sql, test.expected) - } -} - func TestExplainAnalyzeCTEMemoryAndDiskInfo(t *testing.T) { store := testkit.CreateMockStore(t) tk := testkit.NewTestKit(t, store) diff --git a/executor/test/explain/BUILD.bazel b/executor/test/explain/BUILD.bazel new file mode 100644 index 0000000000000..54112cc1442c3 --- /dev/null +++ b/executor/test/explain/BUILD.bazel @@ -0,0 +1,15 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_test") + +go_test( + name = "explain_test", + srcs = [ + "explain_test.go", + "main_test.go", + ], + deps = [ + "//config", + "//testkit", + "@com_github_stretchr_testify//require", + "@org_uber_go_goleak//:goleak", + ], +) diff --git a/executor/test/explain/explain_test.go b/executor/test/explain/explain_test.go new file mode 100644 index 0000000000000..3a92b8dbc6b61 --- /dev/null +++ b/executor/test/explain/explain_test.go @@ -0,0 +1,117 @@ +// 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 explain + +import ( + "fmt" + "testing" + "time" + + "github.com/pingcap/tidb/config" + "github.com/pingcap/tidb/testkit" + "github.com/stretchr/testify/require" +) + +func TestCheckActRowsWithUnistore(t *testing.T) { + defer config.RestoreFunc()() + config.UpdateGlobal(func(conf *config.Config) { + conf.EnableCollectExecutionInfo = true + }) + + store := testkit.CreateMockStore(t) + // testSuite1 use default mockstore which is unistore + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + tk.MustExec("set tidb_cost_model_version=2") + tk.MustExec("drop table if exists t_unistore_act_rows") + tk.MustExec("create table t_unistore_act_rows(a int, b int, index(a, b))") + tk.MustExec("insert into t_unistore_act_rows values (1, 0), (1, 0), (2, 0), (2, 1)") + tk.MustExec("analyze table t_unistore_act_rows") + tk.MustExec("set @@tidb_merge_join_concurrency= 5;") + + time.Sleep(time.Second * 5) + type testStruct struct { + sql string + expected []string + } + + tests := []testStruct{ + { + sql: "select * from t_unistore_act_rows", + expected: []string{"4", "4"}, + }, + { + sql: "select * from t_unistore_act_rows where a > 1", + expected: []string{"2", "2"}, + }, + { + sql: "select * from t_unistore_act_rows where a > 1 and b > 0", + expected: []string{"1", "1", "2"}, + }, + { + sql: "select b from t_unistore_act_rows", + expected: []string{"4", "4"}, + }, + { + sql: "select * from t_unistore_act_rows where b > 0", + expected: []string{"1", "1", "4"}, + }, + { + sql: "select count(*) from t_unistore_act_rows", + expected: []string{"1", "1", "1", "4"}, + }, + { + sql: "select count(*) from t_unistore_act_rows group by a", + expected: []string{"2", "2", "2", "4"}, + }, + { + sql: "select count(*) from t_unistore_act_rows group by b", + expected: []string{"2", "4", "4"}, + }, + { + sql: "with cte(a) as (select a from t_unistore_act_rows) select (select 1 from cte limit 1) from cte;", + expected: []string{"4", "1", "1", "1", "4", "4", "4", "4", "4"}, + }, + { + sql: "select a, row_number() over (partition by b) from t_unistore_act_rows;", + expected: []string{"4", "4", "4", "4", "4", "4", "4"}, + }, + { + sql: "select /*+ merge_join(t1, t2) */ * from t_unistore_act_rows t1 join t_unistore_act_rows t2 on t1.b = t2.b;", + expected: []string{"10", "10", "4", "4", "4", "4", "4", "4", "4", "4", "4", "4"}, + }, + } + + // Default RPC encoding may cause statistics explain result differ and then the test unstable. + tk.MustExec("set @@tidb_enable_chunk_rpc = on") + + for _, test := range tests { + checkActRows(t, tk, test.sql, test.expected) + } +} + +func checkActRows(t *testing.T, tk *testkit.TestKit, sql string, expected []string) { + actRowsCol := 2 + rows := tk.MustQuery("explain analyze " + sql).Rows() + require.Equal(t, len(expected), len(rows)) + for id, row := range rows { + strs := make([]string, len(row)) + for i, c := range row { + strs[i] = c.(string) + } + + require.Equal(t, expected[id], strs[actRowsCol], fmt.Sprintf("error comparing %s", sql)) + } +} diff --git a/executor/test/explain/main_test.go b/executor/test/explain/main_test.go new file mode 100644 index 0000000000000..365c88b4af771 --- /dev/null +++ b/executor/test/explain/main_test.go @@ -0,0 +1,30 @@ +// Copyright 2022 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 explain + +import ( + "testing" + + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + opts := []goleak.Option{ + goleak.IgnoreTopFunction("github.com/golang/glog.(*loggingT).flushDaemon"), + goleak.IgnoreTopFunction("github.com/lestrrat-go/httprc.runFetchWorker"), + goleak.IgnoreTopFunction("github.com/tikv/client-go/v2/txnkv/transaction.keepAlive"), + } + goleak.VerifyTestMain(m, opts...) +}