Skip to content

Commit

Permalink
executor, types: fix skipped replace when the value is treated as the…
Browse files Browse the repository at this point in the history
… same in CI collations (#20858)
  • Loading branch information
TszKitLo40 authored Nov 11, 2020
1 parent d184120 commit 7ab3649
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 61 deletions.
25 changes: 24 additions & 1 deletion executor/replace.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ import (
"time"

"github.com/pingcap/errors"
"github.com/pingcap/parser/charset"
"github.com/pingcap/parser/mysql"
"github.com/pingcap/tidb/kv"
"github.com/pingcap/tidb/sessionctx/stmtctx"
"github.com/pingcap/tidb/tablecodec"
"github.com/pingcap/tidb/types"
"github.com/pingcap/tidb/util/chunk"
Expand Down Expand Up @@ -72,7 +74,7 @@ func (e *ReplaceExec) removeRow(ctx context.Context, txn kv.Transaction, handle
return false, err
}

rowUnchanged, err := types.EqualDatums(e.ctx.GetSessionVars().StmtCtx, oldRow, newRow)
rowUnchanged, err := e.EqualDatumsAsBinary(e.ctx.GetSessionVars().StmtCtx, oldRow, newRow)
if err != nil {
return false, err
}
Expand All @@ -89,6 +91,27 @@ func (e *ReplaceExec) removeRow(ctx context.Context, txn kv.Transaction, handle
return false, nil
}

// EqualDatumsAsBinary compare if a and b contains the same datum values in binary collation.
func (e *ReplaceExec) EqualDatumsAsBinary(sc *stmtctx.StatementContext, a []types.Datum, b []types.Datum) (bool, error) {
if len(a) != len(b) {
return false, nil
}
for i, ai := range a {
collation := ai.Collation()
// We should use binary collation to compare datum, otherwise the result will be incorrect
ai.SetCollation(charset.CollationBin)
v, err := ai.CompareDatum(sc, &b[i])
ai.SetCollation(collation)
if err != nil {
return false, errors.Trace(err)
}
if v != 0 {
return false, nil
}
}
return true, nil
}

// replaceRow removes all duplicate rows for one row, then inserts it.
func (e *ReplaceExec) replaceRow(ctx context.Context, r toBeCheckedRow) error {
txn, err := e.ctx.Txn(true)
Expand Down
53 changes: 53 additions & 0 deletions executor/write_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/pingcap/tidb/planner/core"
"github.com/pingcap/tidb/session"
"github.com/pingcap/tidb/sessionctx"
"github.com/pingcap/tidb/sessionctx/stmtctx"
"github.com/pingcap/tidb/store/mockstore"
"github.com/pingcap/tidb/table/tables"
"github.com/pingcap/tidb/types"
Expand Down Expand Up @@ -2978,3 +2979,55 @@ func (s *testSerialSuite) TestIssue20724(c *C) {
tk.MustQuery("select * from t1").Check(testkit.Rows("A"))
tk.MustExec("drop table t1")
}

func (s *testSerialSuite) TestIssue20840(c *C) {
collate.SetNewCollationEnabledForTest(true)
defer collate.SetNewCollationEnabledForTest(false)

tk := testkit.NewTestKitWithInit(c, s.store)
tk.MustExec("use test")
tk.MustExec("drop table if exists t1")
tk.MustExec("set tidb_enable_clustered_index = 0")
tk.MustExec("create table t1 (i varchar(20) unique key) collate=utf8mb4_general_ci")
tk.MustExec("insert into t1 values ('a')")
tk.MustExec("replace into t1 values ('A')")
tk.MustQuery("select * from t1").Check(testkit.Rows("A"))
tk.MustExec("drop table t1")
}

func (s *testSuite) TestEqualDatumsAsBinary(c *C) {
tests := []struct {
a []interface{}
b []interface{}
same bool
}{
// Positive cases
{[]interface{}{1}, []interface{}{1}, true},
{[]interface{}{1, "aa"}, []interface{}{1, "aa"}, true},
{[]interface{}{1, "aa", 1}, []interface{}{1, "aa", 1}, true},

// negative cases
{[]interface{}{1}, []interface{}{2}, false},
{[]interface{}{1, "a"}, []interface{}{1, "aaaaaa"}, false},
{[]interface{}{1, "aa", 3}, []interface{}{1, "aa", 2}, false},

// Corner cases
{[]interface{}{}, []interface{}{}, true},
{[]interface{}{nil}, []interface{}{nil}, true},
{[]interface{}{}, []interface{}{1}, false},
{[]interface{}{1}, []interface{}{1, 1}, false},
{[]interface{}{nil}, []interface{}{1}, false},
}
for _, tt := range tests {
testEqualDatumsAsBinary(c, tt.a, tt.b, tt.same)
}
}

func testEqualDatumsAsBinary(c *C, a []interface{}, b []interface{}, same bool) {
sc := new(stmtctx.StatementContext)
re := new(executor.ReplaceExec)
sc.IgnoreTruncate = true
res, err := re.EqualDatumsAsBinary(sc, types.MakeDatums(a...), types.MakeDatums(b...))
c.Assert(err, IsNil)
c.Assert(res, Equals, same, Commentf("a: %v, b: %v", a, b))
}
23 changes: 0 additions & 23 deletions types/datum.go
Original file line number Diff line number Diff line change
Expand Up @@ -1962,29 +1962,6 @@ func MaxValueDatum() Datum {
return Datum{k: KindMaxValue}
}

// EqualDatums compare if a and b contains the same datum values.
func EqualDatums(sc *stmtctx.StatementContext, a []Datum, b []Datum) (bool, error) {
if len(a) != len(b) {
return false, nil
}
if a == nil && b == nil {
return true, nil
}
if a == nil || b == nil {
return false, nil
}
for i, ai := range a {
v, err := ai.CompareDatum(sc, &b[i])
if err != nil {
return false, errors.Trace(err)
}
if v != 0 {
return false, nil
}
}
return true, nil
}

// SortDatums sorts a slice of datum.
func SortDatums(sc *stmtctx.StatementContext, datums []Datum) error {
sorter := datumsSorter{datums: datums, sc: sc}
Expand Down
36 changes: 0 additions & 36 deletions types/datum_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,42 +114,6 @@ func (ts *testDatumSuite) TestToBool(c *C) {
c.Assert(err, NotNil)
}

func (ts *testDatumSuite) TestEqualDatums(c *C) {
tests := []struct {
a []interface{}
b []interface{}
same bool
}{
// Positive cases
{[]interface{}{1}, []interface{}{1}, true},
{[]interface{}{1, "aa"}, []interface{}{1, "aa"}, true},
{[]interface{}{1, "aa", 1}, []interface{}{1, "aa", 1}, true},

// negative cases
{[]interface{}{1}, []interface{}{2}, false},
{[]interface{}{1, "a"}, []interface{}{1, "aaaaaa"}, false},
{[]interface{}{1, "aa", 3}, []interface{}{1, "aa", 2}, false},

// Corner cases
{[]interface{}{}, []interface{}{}, true},
{[]interface{}{nil}, []interface{}{nil}, true},
{[]interface{}{}, []interface{}{1}, false},
{[]interface{}{1}, []interface{}{1, 1}, false},
{[]interface{}{nil}, []interface{}{1}, false},
}
for _, tt := range tests {
testEqualDatums(c, tt.a, tt.b, tt.same)
}
}

func testEqualDatums(c *C, a []interface{}, b []interface{}, same bool) {
sc := new(stmtctx.StatementContext)
sc.IgnoreTruncate = true
res, err := EqualDatums(sc, MakeDatums(a...), MakeDatums(b...))
c.Assert(err, IsNil)
c.Assert(res, Equals, same, Commentf("a: %v, b: %v", a, b))
}

func testDatumToInt64(c *C, val interface{}, expect int64) {
d := NewDatum(val)
sc := new(stmtctx.StatementContext)
Expand Down
9 changes: 8 additions & 1 deletion util/profile/flamegraph_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,14 @@ func (s *profileInternalSuite) TestProfileToDatum(c *C) {
c.Assert(err, IsNil, comment)

comment = Commentf("row %2d, actual (%s), expected (%s)", i, rowStr, expectStr)
equal, err := types.EqualDatums(nil, row, datums[i])
equal := true
for j, r := range row {
v, err := r.CompareDatum(nil, &datums[i][j])
if v != 0 || err != nil {
equal = false
break
}
}
c.Assert(err, IsNil, comment)
c.Assert(equal, IsTrue, comment)
}
Expand Down

0 comments on commit 7ab3649

Please sign in to comment.