From 087165624b5ef6c4bc84cdba237d6ad92d4c64fe Mon Sep 17 00:00:00 2001 From: Yiding Cui Date: Fri, 19 Apr 2019 12:58:25 +0800 Subject: [PATCH 1/2] executor: fix the wrong `order by pk desc` result and some corner cases for unsigned pk (#10179) --- executor/analyze.go | 2 +- executor/distsql.go | 32 +++++++++++++++++++++----------- executor/distsql_test.go | 12 ++++++++++++ executor/table_reader.go | 12 +++++++----- 4 files changed, 41 insertions(+), 17 deletions(-) diff --git a/executor/analyze.go b/executor/analyze.go index 06b59b41a7746..521e5a72181bb 100644 --- a/executor/analyze.go +++ b/executor/analyze.go @@ -324,7 +324,7 @@ func (e *AnalyzeColumnsExec) open() error { ranges = ranger.FullIntRange(false) } e.resultHandler = &tableResultHandler{} - firstPartRanges, secondPartRanges := splitRanges(ranges, e.keepOrder) + firstPartRanges, secondPartRanges := splitRanges(ranges, e.keepOrder, false) firstResult, err := e.buildResp(firstPartRanges) if err != nil { return errors.Trace(err) diff --git a/executor/distsql.go b/executor/distsql.go index 3fd656e38c13d..32d509ad13315 100644 --- a/executor/distsql.go +++ b/executor/distsql.go @@ -156,7 +156,7 @@ func handleIsExtra(col *expression.Column) bool { return false } -func splitRanges(ranges []*ranger.Range, keepOrder bool) ([]*ranger.Range, []*ranger.Range) { +func splitRanges(ranges []*ranger.Range, keepOrder bool, desc bool) ([]*ranger.Range, []*ranger.Range) { if len(ranges) == 0 || ranges[0].LowVal[0].Kind() == types.KindInt64 { return ranges, nil } @@ -170,27 +170,37 @@ func splitRanges(ranges []*ranger.Range, keepOrder bool) ([]*ranger.Range, []*ra if !keepOrder { return append(unsignedRanges, signedRanges...), nil } + if desc { + return unsignedRanges, signedRanges + } return signedRanges, unsignedRanges } signedRanges := make([]*ranger.Range, 0, idx+1) unsignedRanges := make([]*ranger.Range, 0, len(ranges)-idx) signedRanges = append(signedRanges, ranges[0:idx]...) - signedRanges = append(signedRanges, &ranger.Range{ - LowVal: ranges[idx].LowVal, - LowExclude: ranges[idx].LowExclude, - HighVal: []types.Datum{types.NewUintDatum(math.MaxInt64)}, - }) - unsignedRanges = append(unsignedRanges, &ranger.Range{ - LowVal: []types.Datum{types.NewUintDatum(math.MaxInt64 + 1)}, - HighVal: ranges[idx].HighVal, - HighExclude: ranges[idx].HighExclude, - }) + if !(ranges[idx].LowVal[0].GetUint64() == math.MaxInt64 && ranges[idx].LowExclude) { + signedRanges = append(signedRanges, &ranger.Range{ + LowVal: ranges[idx].LowVal, + LowExclude: ranges[idx].LowExclude, + HighVal: []types.Datum{types.NewUintDatum(math.MaxInt64)}, + }) + } + if !(ranges[idx].HighVal[0].GetUint64() == math.MaxInt64+1 && ranges[idx].HighExclude) { + unsignedRanges = append(unsignedRanges, &ranger.Range{ + LowVal: []types.Datum{types.NewUintDatum(math.MaxInt64 + 1)}, + HighVal: ranges[idx].HighVal, + HighExclude: ranges[idx].HighExclude, + }) + } if idx < len(ranges) { unsignedRanges = append(unsignedRanges, ranges[idx+1:]...) } if !keepOrder { return append(unsignedRanges, signedRanges...), nil } + if desc { + return unsignedRanges, signedRanges + } return signedRanges, unsignedRanges } diff --git a/executor/distsql_test.go b/executor/distsql_test.go index c4b29c80071ca..2e34acc8a96a5 100644 --- a/executor/distsql_test.go +++ b/executor/distsql_test.go @@ -207,3 +207,15 @@ func (s *testSuite) TestUniqueKeyNullValueSelect(c *C) { res = tk.MustQuery("select * from t where id is null;") res.Check(testkit.Rows(" a", " b", " c")) } + +// TestIssue10178 contains tests for https://github.com/pingcap/tidb/issues/10178 . +func (s *testSuite3) TestIssue10178(c *C) { + tk := testkit.NewTestKit(c, s.store) + tk.MustExec("use test") + tk.MustExec("drop table if exists t") + tk.MustExec("create table t(a bigint unsigned primary key)") + tk.MustExec("insert into t values(9223372036854775807), (18446744073709551615)") + tk.MustQuery("select max(a) from t").Check(testkit.Rows("18446744073709551615")) + tk.MustQuery("select * from t where a > 9223372036854775807").Check(testkit.Rows("18446744073709551615")) + tk.MustQuery("select * from t where a < 9223372036854775808").Check(testkit.Rows("9223372036854775807")) +} diff --git a/executor/table_reader.go b/executor/table_reader.go index 45aeef858860d..c8be63fa3fa95 100644 --- a/executor/table_reader.go +++ b/executor/table_reader.go @@ -96,7 +96,7 @@ func (e *TableReaderExecutor) Open(ctx context.Context) error { } e.resultHandler = &tableResultHandler{} - firstPartRanges, secondPartRanges := splitRanges(e.ranges, e.keepOrder) + firstPartRanges, secondPartRanges := splitRanges(e.ranges, e.keepOrder, e.desc) firstResult, err := e.buildResp(ctx, firstPartRanges) if err != nil { e.feedback.Invalidate() @@ -160,10 +160,12 @@ func (e *TableReaderExecutor) buildResp(ctx context.Context, ranges []*ranger.Ra } type tableResultHandler struct { - // If the pk is unsigned and we have KeepOrder=true. - // optionalResult handles the request whose range is in signed int range. - // result handles the request whose range is exceed signed int range. - // Otherwise, we just set optionalFinished true and the result handles the whole ranges. + // If the pk is unsigned and we have KeepOrder=true and want ascending order, + // `optionalResult` will handles the request whose range is in signed int range, and + // `result` will handle the request whose range is exceed signed int range. + // If we want descending order, `optionalResult` will handles the request whose range is exceed signed, and + // the `result` will handle the request whose range is in signed. + // Otherwise, we just set `optionalFinished` true and the `result` handles the whole ranges. optionalResult distsql.SelectResult result distsql.SelectResult From d217795daf847fe59dc253acebe20596c2398ffe Mon Sep 17 00:00:00 2001 From: Yiding Cui Date: Mon, 22 Apr 2019 13:38:38 +0800 Subject: [PATCH 2/2] fix unit test --- executor/distsql_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/executor/distsql_test.go b/executor/distsql_test.go index 2e34acc8a96a5..804782c591e0d 100644 --- a/executor/distsql_test.go +++ b/executor/distsql_test.go @@ -209,7 +209,7 @@ func (s *testSuite) TestUniqueKeyNullValueSelect(c *C) { } // TestIssue10178 contains tests for https://github.com/pingcap/tidb/issues/10178 . -func (s *testSuite3) TestIssue10178(c *C) { +func (s *testSuite) TestIssue10178(c *C) { tk := testkit.NewTestKit(c, s.store) tk.MustExec("use test") tk.MustExec("drop table if exists t")