From 09f9023e55f1f33b62cf04dccdf03fae2b45b3aa Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Sat, 10 Sep 2022 06:19:49 +0800 Subject: [PATCH 1/9] add comment --- dbms/src/Functions/FunctionsLogical.h | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/dbms/src/Functions/FunctionsLogical.h b/dbms/src/Functions/FunctionsLogical.h index d1a50fbf644..2b30015e4ba 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -27,6 +27,9 @@ #include +#include +#include + namespace DB { @@ -199,7 +202,24 @@ struct AssociativeOperationImpl if (Op::isSaturable()) { UInt8 a = vec[i]; - return Op::isSaturatedValue(a) ? a : continuation.apply(i); + + // !!a is a hack + // TiFlash converts columns with non-UInt8 type to UInt8 type and sets value to 0 or 1 + // which correspond to false or true. However, for columns with UInt8 type, + // no more convertion will be executed on them and the values stored + // in them are 'origin' which means that they won't be converted to 0 or 1. + // For example: + // Input column with non-UInt8 type: + // column_values = {-2, 0, 2} + // then, we will get vec + // vec = {1, 0, 1} (here vec stores converted values) + // + // Input column with UInt8 type: + // column_values = {1, 0, 2} + // then, we will get vec + // vec = {1, 0, 2} (error, we only want 0 or 1) + // See issue: https://github.com/pingcap/tidb/issues/37258 + return Op::isSaturatedValue(a) ? !!a : continuation.apply(i); } else { @@ -402,6 +422,8 @@ class FunctionAnyArityLogical : public IFunction void executeImpl(Block & block, const ColumnNumbers & arguments, size_t result) const override { + auto log = &Poco::Logger::get("LRUCache"); + LOG_FMT_INFO(log, "or function is executed."); bool has_nullable_input_column = false; size_t num_arguments = arguments.size(); From 1afeb102a99f4e02334ae16330c576c791fe7642 Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Sat, 10 Sep 2022 13:06:24 +0800 Subject: [PATCH 2/9] add test --- dbms/src/Functions/FunctionsLogical.h | 6 +++--- tests/fullstack-test/expr/logical_op.test | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/dbms/src/Functions/FunctionsLogical.h b/dbms/src/Functions/FunctionsLogical.h index 2b30015e4ba..e3428d67f57 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -203,7 +203,7 @@ struct AssociativeOperationImpl { UInt8 a = vec[i]; - // !!a is a hack + // !!a is a trick // TiFlash converts columns with non-UInt8 type to UInt8 type and sets value to 0 or 1 // which correspond to false or true. However, for columns with UInt8 type, // no more convertion will be executed on them and the values stored @@ -211,12 +211,12 @@ struct AssociativeOperationImpl // For example: // Input column with non-UInt8 type: // column_values = {-2, 0, 2} - // then, we will get vec + // then, they will be converted to: // vec = {1, 0, 1} (here vec stores converted values) // // Input column with UInt8 type: // column_values = {1, 0, 2} - // then, we will get vec + // then, the vec will be: // vec = {1, 0, 2} (error, we only want 0 or 1) // See issue: https://github.com/pingcap/tidb/issues/37258 return Op::isSaturatedValue(a) ? !!a : continuation.apply(i); diff --git a/tests/fullstack-test/expr/logical_op.test b/tests/fullstack-test/expr/logical_op.test index a77aca0ca3b..d3ccf9a9b61 100644 --- a/tests/fullstack-test/expr/logical_op.test +++ b/tests/fullstack-test/expr/logical_op.test @@ -18,16 +18,20 @@ mysql> drop table if exists test.t3; mysql> create table test.t1(a char(20),b double); mysql> create table test.t2(a char(20)); mysql> create table test.t3(a int); +mysql> create table test.t4(a tinyint(45) unsigned NOT NULL, b bigint(20) NOT NULL); mysql> insert into test.t1 values(1,null),('j',0),(1,12.991),(0,0),(0,0),('they',1.009),('can',-99),(0,12.991),(1,-9.183),(null,1); mysql> insert into test.t2 values(0),(0),(0),(0),(0),(0),(1),('with'),('see'),(null); mysql> insert into test.t3 values(0),(1); +mysql> insert into test.t4 values(65, 1),(66, 2), (67, 3), (0, 0); mysql> alter table test.t1 set tiflash replica 1; mysql> alter table test.t2 set tiflash replica 1; mysql> alter table test.t3 set tiflash replica 1; +mysql> alter table test.t4 set tiflash replica 1; func> wait_table test t1 func> wait_table test t2 func> wait_table test t3 +func> wait_table test t4 mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t1 where (b between null and 100) is null; +----------+ @@ -81,6 +85,18 @@ mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select count(*) from test.t3 group by a having ifnull(null,count(*)) and min(null); # empty +# issue 37258 +mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select a or b from test.t4; ++--------+ +| a or b | ++--------+ +| 1 | +| 1 | +| 1 | +| 0 | ++--------+ + #mysql> drop table if exists test.t1; #mysql> drop table if exists test.t2; #mysql> drop table if exists test.t3; +#mysql> drop table if exists test.t4; From 424d32cc84d34afbbc19a499857f614398e94a65 Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Sat, 10 Sep 2022 13:14:14 +0800 Subject: [PATCH 3/9] remove useless codes --- dbms/src/Functions/FunctionsLogical.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/dbms/src/Functions/FunctionsLogical.h b/dbms/src/Functions/FunctionsLogical.h index e3428d67f57..76d1da5b2fd 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -27,9 +27,6 @@ #include -#include -#include - namespace DB { @@ -422,8 +419,6 @@ class FunctionAnyArityLogical : public IFunction void executeImpl(Block & block, const ColumnNumbers & arguments, size_t result) const override { - auto log = &Poco::Logger::get("LRUCache"); - LOG_FMT_INFO(log, "or function is executed."); bool has_nullable_input_column = false; size_t num_arguments = arguments.size(); From a013f010d03f80a5a52c6d8b27b8cdf59fdaf91e Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Tue, 13 Sep 2022 09:03:03 +0800 Subject: [PATCH 4/9] format --- dbms/src/Functions/FunctionsLogical.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dbms/src/Functions/FunctionsLogical.h b/dbms/src/Functions/FunctionsLogical.h index 76d1da5b2fd..4ea6d26a0ce 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -201,8 +201,8 @@ struct AssociativeOperationImpl UInt8 a = vec[i]; // !!a is a trick - // TiFlash converts columns with non-UInt8 type to UInt8 type and sets value to 0 or 1 - // which correspond to false or true. However, for columns with UInt8 type, + // TiFlash converts columns with non-UInt8 type to UInt8 type and sets value to 0 or 1 + // which correspond to false or true. However, for columns with UInt8 type, // no more convertion will be executed on them and the values stored // in them are 'origin' which means that they won't be converted to 0 or 1. // For example: From 48a52581c1d3c94da54e9f9d181127a218f80b6b Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Tue, 13 Sep 2022 09:36:22 +0800 Subject: [PATCH 5/9] tweaking --- dbms/src/Functions/FunctionsLogical.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/dbms/src/Functions/FunctionsLogical.h b/dbms/src/Functions/FunctionsLogical.h index 4ea6d26a0ce..51a19fb451e 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -198,9 +198,7 @@ struct AssociativeOperationImpl { if (Op::isSaturable()) { - UInt8 a = vec[i]; - - // !!a is a trick + // cast a: UInt8 -> bool -> UInt8 is a trick // TiFlash converts columns with non-UInt8 type to UInt8 type and sets value to 0 or 1 // which correspond to false or true. However, for columns with UInt8 type, // no more convertion will be executed on them and the values stored @@ -216,7 +214,8 @@ struct AssociativeOperationImpl // then, the vec will be: // vec = {1, 0, 2} (error, we only want 0 or 1) // See issue: https://github.com/pingcap/tidb/issues/37258 - return Op::isSaturatedValue(a) ? !!a : continuation.apply(i); + bool a = static_cast(vec[i]); + return Op::isSaturatedValue(a) ? static_cast(a) : continuation.apply(i); } else { From 63dc0cff65628b6d32749f1b7190abf3e91bbad8 Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Tue, 13 Sep 2022 20:18:29 +0800 Subject: [PATCH 6/9] add gtest --- dbms/src/Functions/FunctionsLogical.h | 2 +- dbms/src/Functions/tests/gtest_logical.cpp | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/dbms/src/Functions/FunctionsLogical.h b/dbms/src/Functions/FunctionsLogical.h index 51a19fb451e..0adfd699b7e 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -215,7 +215,7 @@ struct AssociativeOperationImpl // vec = {1, 0, 2} (error, we only want 0 or 1) // See issue: https://github.com/pingcap/tidb/issues/37258 bool a = static_cast(vec[i]); - return Op::isSaturatedValue(a) ? static_cast(a) : continuation.apply(i); + return Op::isSaturatedValue(a) ? a : continuation.apply(i); } else { diff --git a/dbms/src/Functions/tests/gtest_logical.cpp b/dbms/src/Functions/tests/gtest_logical.cpp index 7988989cc88..b42154ef80b 100644 --- a/dbms/src/Functions/tests/gtest_logical.cpp +++ b/dbms/src/Functions/tests/gtest_logical.cpp @@ -73,6 +73,13 @@ try func_name, createColumn>({0, 1, 0, 1, {}, 0}), createColumn>({0, 1, 1, 0, 1, {}}))); + // issue 37258 + ASSERT_COLUMN_EQ( + createColumn>({0, 1, 1, 1, 1, {}}), + executeFunction( + func_name, + createColumn>({0, 123, 0, 41, {}, 0}), + createColumn>({0, 11, 221, 0, 11, {}}))); // column, const ASSERT_COLUMN_EQ( createColumn>({1, 1}), From 2f1d924bc8649e4fcefa77f50c9c255ca08e6b0e Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Tue, 13 Sep 2022 20:45:56 +0800 Subject: [PATCH 7/9] fix gtest --- dbms/src/Functions/tests/gtest_logical.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dbms/src/Functions/tests/gtest_logical.cpp b/dbms/src/Functions/tests/gtest_logical.cpp index b42154ef80b..2c5f4127025 100644 --- a/dbms/src/Functions/tests/gtest_logical.cpp +++ b/dbms/src/Functions/tests/gtest_logical.cpp @@ -75,11 +75,11 @@ try createColumn>({0, 1, 1, 0, 1, {}}))); // issue 37258 ASSERT_COLUMN_EQ( - createColumn>({0, 1, 1, 1, 1, {}}), + createColumn({0, 1, 1, 1}), executeFunction( func_name, - createColumn>({0, 123, 0, 41, {}, 0}), - createColumn>({0, 11, 221, 0, 11, {}}))); + createColumn({0, 123, 0, 41}), + createColumn({0, 11, 221, 0}))); // column, const ASSERT_COLUMN_EQ( createColumn>({1, 1}), From cbd4368f0b1da054ecd3ecfbe034efd600c9feeb Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Tue, 13 Sep 2022 20:47:50 +0800 Subject: [PATCH 8/9] fix issue number --- dbms/src/Functions/tests/gtest_logical.cpp | 2 +- tests/fullstack-test/expr/logical_op.test | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dbms/src/Functions/tests/gtest_logical.cpp b/dbms/src/Functions/tests/gtest_logical.cpp index 2c5f4127025..a5bde6ccd2d 100644 --- a/dbms/src/Functions/tests/gtest_logical.cpp +++ b/dbms/src/Functions/tests/gtest_logical.cpp @@ -73,7 +73,7 @@ try func_name, createColumn>({0, 1, 0, 1, {}, 0}), createColumn>({0, 1, 1, 0, 1, {}}))); - // issue 37258 + // issue 5849 ASSERT_COLUMN_EQ( createColumn({0, 1, 1, 1}), executeFunction( diff --git a/tests/fullstack-test/expr/logical_op.test b/tests/fullstack-test/expr/logical_op.test index d3ccf9a9b61..e7882dfa547 100644 --- a/tests/fullstack-test/expr/logical_op.test +++ b/tests/fullstack-test/expr/logical_op.test @@ -85,7 +85,7 @@ mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select count(*) from test.t3 group by a having ifnull(null,count(*)) and min(null); # empty -# issue 37258 +# issue 5849 mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select a or b from test.t4; +--------+ | a or b | From 5b336624998736d5595caad8ab7ad5cd0d9d6f88 Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Wed, 14 Sep 2022 08:04:29 +0800 Subject: [PATCH 9/9] fix integration tests --- tests/fullstack-test/expr/logical_op.test | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/fullstack-test/expr/logical_op.test b/tests/fullstack-test/expr/logical_op.test index e7882dfa547..65e8f32517c 100644 --- a/tests/fullstack-test/expr/logical_op.test +++ b/tests/fullstack-test/expr/logical_op.test @@ -15,6 +15,7 @@ mysql> drop table if exists test.t1; mysql> drop table if exists test.t2; mysql> drop table if exists test.t3; +mysql> drop table if exists test.t4; mysql> create table test.t1(a char(20),b double); mysql> create table test.t2(a char(20)); mysql> create table test.t3(a int);