From eb002463f5eb40f6b8b816665231d2c95ea59988 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Tue, 22 Mar 2022 17:46:33 +0800 Subject: [PATCH] *: fix bug that UnionScan can't keep order caused wrong result (#33218) close pingcap/tidb#33175 --- executor/table_reader.go | 11 ++++ planner/core/handle_cols.go | 12 ++--- planner/core/integration_test.go | 89 ++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 9 deletions(-) diff --git a/executor/table_reader.go b/executor/table_reader.go index 77003b0c7e3cd..a9decdd55b1a7 100644 --- a/executor/table_reader.go +++ b/executor/table_reader.go @@ -181,6 +181,17 @@ func (e *TableReaderExecutor) Open(ctx context.Context) error { // Calculate the kv ranges here, UnionScan rely on this kv ranges. // cached table and temporary table are similar if e.dummy { + if e.desc && len(secondPartRanges) != 0 { + // TiKV support reverse scan and the `resultHandler` process the range order. + // While in UnionScan, it doesn't use reverse scan and reverse the final result rows manually. + // So things are differ, we need to reverse the kv range here. + // TODO: If we refactor UnionScan to use reverse scan, update the code here. + // [9734095886065816708 9734095886065816709] | [1 3] [65535 9734095886065816707] => before the following change + // [1 3] [65535 9734095886065816707] | [9734095886065816708 9734095886065816709] => ranges part reverse here + // [1 3 65535 9734095886065816707 9734095886065816708 9734095886065816709] => scan (normal order) in UnionScan + // [9734095886065816709 9734095886065816708 9734095886065816707 65535 3 1] => rows reverse in UnionScan + firstPartRanges, secondPartRanges = secondPartRanges, firstPartRanges + } kvReq, err := e.buildKVReq(ctx, firstPartRanges) if err != nil { return err diff --git a/planner/core/handle_cols.go b/planner/core/handle_cols.go index 48d6ab2444edd..ad59247a3b69a 100644 --- a/planner/core/handle_cols.go +++ b/planner/core/handle_cols.go @@ -239,15 +239,9 @@ func (ib *IntHandleCols) NumCols() int { // Compare implements the kv.HandleCols interface. func (ib *IntHandleCols) Compare(a, b []types.Datum, ctors []collate.Collator) (int, error) { - aInt := a[ib.col.Index].GetInt64() - bInt := b[ib.col.Index].GetInt64() - if aInt == bInt { - return 0, nil - } - if aInt < bInt { - return -1, nil - } - return 1, nil + aVal := &a[ib.col.Index] + bVal := &b[ib.col.Index] + return aVal.Compare(nil, bVal, ctors[ib.col.Index]) } // GetFieldsTypes implements the kv.HandleCols interface. diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index e87e689d67a56..8a12ddf7c197f 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -6239,3 +6239,92 @@ func TestTiFlashPartitionTableScan(t *testing.T) { tk.MustExec("drop table rp_t;") tk.MustExec("drop table hp_t;") } + +func TestIssue33175(t *testing.T) { + store, _, clean := testkit.CreateMockStoreAndDomain(t) + defer clean() + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + + tk.MustExec("create table t (id bigint(45) unsigned not null, c varchar(20), primary key(id));") + tk.MustExec("insert into t values (9734095886065816707, 'a'), (10353107668348738101, 'b'), (0, 'c');") + tk.MustExec("begin") + tk.MustExec("insert into t values (33, 'd');") + tk.MustQuery("select max(id) from t;").Check(testkit.Rows("10353107668348738101")) + tk.MustExec("rollback") + + tk.MustExec("alter table t cache") + for { + tk.MustQuery("select max(id) from t;").Check(testkit.Rows("10353107668348738101")) + if tk.Session().GetSessionVars().StmtCtx.ReadFromTableCache { + break + } + } + + // // With subquery, like the original issue case. + for { + tk.MustQuery("select * from t where id > (select max(id) from t where t.id > 0);").Check(testkit.Rows()) + if tk.Session().GetSessionVars().StmtCtx.ReadFromTableCache { + break + } + } + + // Test order by desc / asc. + tk.MustQuery("select id from t order by id desc;").Check(testkit.Rows( + "10353107668348738101", + "9734095886065816707", + "0")) + + tk.MustQuery("select id from t order by id asc;").Check(testkit.Rows( + "0", + "9734095886065816707", + "10353107668348738101")) + + tk.MustExec("alter table t nocache") + tk.MustExec("drop table t") + + // Cover more code that use union scan + // TableReader/IndexReader/IndexLookup + for idx, q := range []string{ + "create temporary table t (id bigint unsigned, c int default null, index(id))", + "create temporary table t (id bigint unsigned primary key)", + } { + tk.MustExec(q) + tk.MustExec("insert into t(id) values (1), (3), (9734095886065816707), (9734095886065816708)") + tk.MustQuery("select min(id) from t").Check(testkit.Rows("1")) + tk.MustQuery("select max(id) from t").Check(testkit.Rows("9734095886065816708")) + tk.MustQuery("select id from t order by id asc").Check(testkit.Rows( + "1", "3", "9734095886065816707", "9734095886065816708")) + tk.MustQuery("select id from t order by id desc").Check(testkit.Rows( + "9734095886065816708", "9734095886065816707", "3", "1")) + if idx == 0 { + tk.MustQuery("select * from t order by id asc").Check(testkit.Rows( + "1 ", + "3 ", + "9734095886065816707 ", + "9734095886065816708 ")) + tk.MustQuery("select * from t order by id desc").Check(testkit.Rows( + "9734095886065816708 ", + "9734095886065816707 ", + "3 ", + "1 ")) + } + tk.MustExec("drop table t") + } + + // More and more test + tk.MustExec("create global temporary table `tmp1` (id bigint unsigned primary key) on commit delete rows;") + tk.MustExec("begin") + tk.MustExec("insert into tmp1 values (0),(1),(2),(65536),(9734095886065816707),(9734095886065816708);") + tk.MustQuery("select * from tmp1 where id <= 65534 or (id > 65535 and id < 9734095886065816700) or id >= 9734095886065816707 order by id desc;").Check(testkit.Rows( + "9734095886065816708", "9734095886065816707", "65536", "2", "1", "0")) + + tk.MustQuery("select * from tmp1 where id <= 65534 or (id > 65535 and id < 9734095886065816700) or id >= 9734095886065816707 order by id asc;").Check(testkit.Rows( + "0", "1", "2", "65536", "9734095886065816707", "9734095886065816708")) + + tk.MustExec("create global temporary table `tmp2` (id bigint primary key) on commit delete rows;") + tk.MustExec("begin") + tk.MustExec("insert into tmp2 values(-2),(-1),(0),(1),(2);") + tk.MustQuery("select * from tmp2 where id <= -1 or id > 0 order by id desc;").Check(testkit.Rows("2", "1", "-1", "-2")) + tk.MustQuery("select * from tmp2 where id <= -1 or id > 0 order by id asc;").Check(testkit.Rows("-2", "-1", "1", "2")) +}