From 4ed280e8816a301fba770f86052871bf840807de Mon Sep 17 00:00:00 2001 From: Chao Wang Date: Tue, 19 Nov 2024 11:15:23 +0800 Subject: [PATCH 1/4] expression: fix tikv crash when `bool like cast(bit as char)` --- pkg/expression/builtin_cast.go | 18 ++++++++++++++---- pkg/expression/builtin_convert_charset.go | 1 + pkg/expression/expr_to_pb_test.go | 9 +++++++-- tests/integrationtest/r/expression/cast.result | 15 +++++++++++++++ tests/integrationtest/t/expression/cast.test | 11 +++++++++++ 5 files changed, 48 insertions(+), 6 deletions(-) diff --git a/pkg/expression/builtin_cast.go b/pkg/expression/builtin_cast.go index f4419dc96c4b5..c1a9aec8eae50 100644 --- a/pkg/expression/builtin_cast.go +++ b/pkg/expression/builtin_cast.go @@ -300,10 +300,20 @@ func (c *castAsStringFunctionClass) getFunction(ctx BuildContext, args []Express if err != nil { return nil, err } - if args[0].GetType(ctx.GetEvalCtx()).Hybrid() { - sig = &builtinCastStringAsStringSig{bf} - sig.setPbCode(tipb.ScalarFuncSig_CastStringAsString) - return sig, nil + if ft := args[0].GetType(ctx.GetEvalCtx()); ft.Hybrid() { + castBitAsUnBinary := ft.GetType() == mysql.TypeBit && c.tp.GetCharset() != charset.CharsetBin + if !castBitAsUnBinary { + sig = &builtinCastStringAsStringSig{bf} + sig.setPbCode(tipb.ScalarFuncSig_CastStringAsString) + return sig, nil + } + // for type BIT, it maybe an invalid value for the specified charset, we need to convert it to binary first, + // and then convert it to the specified charset with `HandleBinaryLiteral` in the following code. + tp := types.NewFieldType(mysql.TypeString) + tp.SetCharset(charset.CharsetBin) + tp.SetCollate(charset.CollationBin) + tp.AddFlag(mysql.BinaryFlag) + args[0] = BuildCastFunction(ctx, args[0], tp) } argTp := args[0].GetType(ctx.GetEvalCtx()).EvalType() switch argTp { diff --git a/pkg/expression/builtin_convert_charset.go b/pkg/expression/builtin_convert_charset.go index 24b4acd054bd6..33524d80dcdb7 100644 --- a/pkg/expression/builtin_convert_charset.go +++ b/pkg/expression/builtin_convert_charset.go @@ -173,6 +173,7 @@ type builtinInternalFromBinarySig struct { func (b *builtinInternalFromBinarySig) Clone() builtinFunc { newSig := &builtinInternalFromBinarySig{} newSig.cloneFrom(&b.baseBuiltinFunc) + newSig.cannotConvertStringAsWarning = b.cannotConvertStringAsWarning return newSig } diff --git a/pkg/expression/expr_to_pb_test.go b/pkg/expression/expr_to_pb_test.go index f407372c3ded6..c45e7beda75b0 100644 --- a/pkg/expression/expr_to_pb_test.go +++ b/pkg/expression/expr_to_pb_test.go @@ -1581,7 +1581,8 @@ func TestExprPushDownToTiKV(t *testing.T) { require.Len(t, pushed, 0) require.Len(t, remained, len(exprs)) - // Test Conv function + // Test Conv function, `conv` function for a BIT column should not be pushed down for its special behavior which + // is only handled in TiDB currently. exprs = exprs[:0] function, err = NewFunction(mock.NewContext(), ast.Conv, types.NewFieldType(mysql.TypeString), stringColumn, intColumn, intColumn) require.NoError(t, err) @@ -1590,7 +1591,11 @@ func TestExprPushDownToTiKV(t *testing.T) { require.Len(t, pushed, len(exprs)) require.Len(t, remained, 0) exprs = exprs[:0] - castByteAsStringFunc, err := NewFunction(mock.NewContext(), ast.Cast, types.NewFieldType(mysql.TypeString), byteColumn) + // when conv a column with type BIT, a cast function will be used to cast bit to a binary string + castTp := types.NewFieldType(mysql.TypeString) + castTp.SetCharset(charset.CharsetBin) + castTp.SetCollate(charset.CollationBin) + castByteAsStringFunc, err := NewFunction(mock.NewContext(), ast.Cast, castTp, byteColumn) require.NoError(t, err) function, err = NewFunction(mock.NewContext(), ast.Conv, types.NewFieldType(mysql.TypeString), castByteAsStringFunc, intColumn, intColumn) require.NoError(t, err) diff --git a/tests/integrationtest/r/expression/cast.result b/tests/integrationtest/r/expression/cast.result index 1d237ebfd87e0..147b93ee4260c 100644 --- a/tests/integrationtest/r/expression/cast.result +++ b/tests/integrationtest/r/expression/cast.result @@ -178,3 +178,18 @@ id update_user 2 李四 4 李四 1 张三 +drop table if exists test; +create table test(a bit(24)); +insert into test values('中'); +select a from test where '中' like convert(a, char); +a +中 +select a from test where false not like convert(a, char); +a +中 +select a from test where false like convert(a, char); +a +truncate table test; +insert into test values(0xffffff); +select a from test where false not like convert(a, char); +Error 1105 (HY000): Cannot convert string '\xFF\xFF\xFF' from binary to utf8mb4 diff --git a/tests/integrationtest/t/expression/cast.test b/tests/integrationtest/t/expression/cast.test index 7af48437383a9..cc5926168b1e0 100644 --- a/tests/integrationtest/t/expression/cast.test +++ b/tests/integrationtest/t/expression/cast.test @@ -113,3 +113,14 @@ insert into test values(3,'张三'); insert into test values(4,'李四'); select * from test order by cast(update_user as char) desc , id limit 3; +# issue #56494, cast bit as char +drop table if exists test; +create table test(a bit(24)); +insert into test values('中'); +select a from test where '中' like convert(a, char); +select a from test where false not like convert(a, char); +select a from test where false like convert(a, char); +truncate table test; +insert into test values(0xffffff); +-- error 1105 +select a from test where false not like convert(a, char); From 48abcf75106ca5b28dde38579a8635d68232da2e Mon Sep 17 00:00:00 2001 From: Chao Wang Date: Tue, 19 Nov 2024 13:52:29 +0800 Subject: [PATCH 2/4] add realtikv test --- tests/realtikvtest/pushdowntest/BUILD.bazel | 16 +++++++++ tests/realtikvtest/pushdowntest/expr_test.go | 36 ++++++++++++++++++++ tests/realtikvtest/pushdowntest/main_test.go | 25 ++++++++++++++ 3 files changed, 77 insertions(+) create mode 100644 tests/realtikvtest/pushdowntest/BUILD.bazel create mode 100644 tests/realtikvtest/pushdowntest/expr_test.go create mode 100644 tests/realtikvtest/pushdowntest/main_test.go diff --git a/tests/realtikvtest/pushdowntest/BUILD.bazel b/tests/realtikvtest/pushdowntest/BUILD.bazel new file mode 100644 index 0000000000000..5128eab1cb102 --- /dev/null +++ b/tests/realtikvtest/pushdowntest/BUILD.bazel @@ -0,0 +1,16 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_test") + +go_test( + name = "pushdowntest_test", + timeout = "short", + srcs = [ + "expr_test.go", + "main_test.go", + ], + flaky = True, + deps = [ + "//pkg/testkit", + "//tests/realtikvtest", + "@com_github_stretchr_testify//require", + ], +) diff --git a/tests/realtikvtest/pushdowntest/expr_test.go b/tests/realtikvtest/pushdowntest/expr_test.go new file mode 100644 index 0000000000000..110d240b8da43 --- /dev/null +++ b/tests/realtikvtest/pushdowntest/expr_test.go @@ -0,0 +1,36 @@ +// Copyright 2024 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 pushdowntest + +import ( + "testing" + + "github.com/pingcap/tidb/pkg/testkit" + "github.com/pingcap/tidb/tests/realtikvtest" + "github.com/stretchr/testify/require" +) + +// TestBitCastInTiKV see issue: https://github.com/pingcap/tidb/issues/56494 +func TestBitCastInTiKV(t *testing.T) { + store := realtikvtest.CreateMockStoreAndSetup(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + tk.MustExec("drop table if exists t1") + defer tk.MustExec("drop table if exists t1") + tk.MustExec("create table t1(a bit(24))") + tk.MustExec("insert into t1 values(0xffffff)") + err := tk.QueryToErr("select a from t1 where false not like convert(a, char)") + require.EqualError(t, err, "[tikv:3854]Cannot convert string '\\xFF\\xFF\\xFF' from binary to utf8mb4") +} diff --git a/tests/realtikvtest/pushdowntest/main_test.go b/tests/realtikvtest/pushdowntest/main_test.go new file mode 100644 index 0000000000000..fc34188dac29d --- /dev/null +++ b/tests/realtikvtest/pushdowntest/main_test.go @@ -0,0 +1,25 @@ +// Copyright 2024 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 pushdowntest + +import ( + "testing" + + "github.com/pingcap/tidb/tests/realtikvtest" +) + +func TestMain(m *testing.M) { + realtikvtest.RunTestMain(m) +} From df5cace6b4ebc20f9279d8e010f88d384a6915fc Mon Sep 17 00:00:00 2001 From: Chao Wang Date: Tue, 19 Nov 2024 15:40:34 +0800 Subject: [PATCH 3/4] fix some tests --- pkg/planner/core/integration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/planner/core/integration_test.go b/pkg/planner/core/integration_test.go index 4002325148e25..38837cc33b38e 100644 --- a/pkg/planner/core/integration_test.go +++ b/pkg/planner/core/integration_test.go @@ -309,7 +309,7 @@ func TestBitColumnPushDown(t *testing.T) { tk.MustQuery(sql).Check(testkit.Rows("A")) tk.MustQuery(fmt.Sprintf("explain analyze %s", sql)).CheckAt([]int{0, 3, 6}, rows) - rows[1][2] = `eq(cast(test.t1.a, var_string(1)), "A")` + rows[1][2] = `eq(cast(from_binary(cast(test.t1.a, binary(1))), var_string(1)), "A")` sql = "select a from t1 where cast(a as char)='A'" tk.MustQuery(sql).Check(testkit.Rows("A")) tk.MustQuery(fmt.Sprintf("explain analyze %s", sql)).CheckAt([]int{0, 3, 6}, rows) @@ -317,7 +317,7 @@ func TestBitColumnPushDown(t *testing.T) { tk.MustExec("insert into mysql.expr_pushdown_blacklist values('bit', 'tikv','');") tk.MustExec("admin reload expr_pushdown_blacklist;") rows = [][]any{ - {"Selection_5", "root", `eq(cast(test.t1.a, var_string(1)), "A")`}, + {"Selection_5", "root", `eq(cast(from_binary(cast(test.t1.a, binary(1))), var_string(1)), "A")`}, {"└─TableReader_7", "root", "data:TableFullScan_6"}, {" └─TableFullScan_6", "cop[tikv]", "keep order:false, stats:pseudo"}, } From 561c9d1f61ca3047248c5fe8d63220e8aa484d4d Mon Sep 17 00:00:00 2001 From: Chao Wang Date: Wed, 20 Nov 2024 11:35:32 +0800 Subject: [PATCH 4/4] add some comment --- pkg/expression/expr_to_pb_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/expression/expr_to_pb_test.go b/pkg/expression/expr_to_pb_test.go index c45e7beda75b0..f6d9d94327999 100644 --- a/pkg/expression/expr_to_pb_test.go +++ b/pkg/expression/expr_to_pb_test.go @@ -1583,6 +1583,7 @@ func TestExprPushDownToTiKV(t *testing.T) { // Test Conv function, `conv` function for a BIT column should not be pushed down for its special behavior which // is only handled in TiDB currently. + // see issue: https://github.com/pingcap/tidb/issues/51877 exprs = exprs[:0] function, err = NewFunction(mock.NewContext(), ast.Conv, types.NewFieldType(mysql.TypeString), stringColumn, intColumn, intColumn) require.NoError(t, err)