From 58cf2343c2d0bff15d38f0ef8c058d6816a4d903 Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Sat, 10 Sep 2022 06:19:49 +0800 Subject: [PATCH 01/10] 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 fe900ea2c5a..42bdfb1c0db 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -13,6 +13,9 @@ #include +#include +#include + namespace DB { @@ -185,7 +188,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 { @@ -388,6 +408,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 81ebb61961556413aae28d38c698d5c5364875ef Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Sat, 10 Sep 2022 13:06:24 +0800 Subject: [PATCH 02/10] 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 42bdfb1c0db..98198800bf7 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -189,7 +189,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 @@ -197,12 +197,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 b64b4ff35e3..56d49d1345d 100644 --- a/tests/fullstack-test/expr/logical_op.test +++ b/tests/fullstack-test/expr/logical_op.test @@ -4,16 +4,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; +----------+ @@ -67,6 +71,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 94c338f2efec9a5f59968e621184a3f98f165db5 Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Sat, 10 Sep 2022 13:14:14 +0800 Subject: [PATCH 03/10] 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 98198800bf7..d2968be8816 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -13,9 +13,6 @@ #include -#include -#include - namespace DB { @@ -408,8 +405,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 e60545354b7a224f2cd89490baf4f12915ea842a Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Tue, 13 Sep 2022 09:03:03 +0800 Subject: [PATCH 04/10] 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 d2968be8816..c4ae4ae8eb7 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -187,8 +187,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 35341000102fac2bf8a213c1531f11e30e1b0888 Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Tue, 13 Sep 2022 09:36:22 +0800 Subject: [PATCH 05/10] 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 c4ae4ae8eb7..4b81ebf7df9 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -184,9 +184,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 @@ -202,7 +200,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 18694af36036ee506675324b01d0a16dd9403cbb Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Tue, 13 Sep 2022 20:18:29 +0800 Subject: [PATCH 06/10] 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 4b81ebf7df9..7f1813dd219 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -201,7 +201,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 5d82d2c8b0b..9dbe52bd8ad 100644 --- a/dbms/src/Functions/tests/gtest_logical.cpp +++ b/dbms/src/Functions/tests/gtest_logical.cpp @@ -59,6 +59,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 d12d48462b51436d5d7c86b52786c4eb96b345c4 Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Tue, 13 Sep 2022 20:45:56 +0800 Subject: [PATCH 07/10] 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 9dbe52bd8ad..6af40f63c8e 100644 --- a/dbms/src/Functions/tests/gtest_logical.cpp +++ b/dbms/src/Functions/tests/gtest_logical.cpp @@ -61,11 +61,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 af770c6f25c963ea659417c217f20b0d0a6ecaaf Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Tue, 13 Sep 2022 20:47:50 +0800 Subject: [PATCH 08/10] 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 6af40f63c8e..1b30e83cf5b 100644 --- a/dbms/src/Functions/tests/gtest_logical.cpp +++ b/dbms/src/Functions/tests/gtest_logical.cpp @@ -59,7 +59,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 56d49d1345d..12b3c3089fe 100644 --- a/tests/fullstack-test/expr/logical_op.test +++ b/tests/fullstack-test/expr/logical_op.test @@ -71,7 +71,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 3487bb64915a562559f300e856556487ae3eec9b Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Wed, 14 Sep 2022 08:04:29 +0800 Subject: [PATCH 09/10] 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 12b3c3089fe..438f881d2cb 100644 --- a/tests/fullstack-test/expr/logical_op.test +++ b/tests/fullstack-test/expr/logical_op.test @@ -1,6 +1,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); From 55060db22aa2c536d61e9b27d1b7428161282412 Mon Sep 17 00:00:00 2001 From: xzhangxian1008 Date: Wed, 14 Sep 2022 15:23:38 +0800 Subject: [PATCH 10/10] fix format file --- format-diff.py | 80 +++++++++++++++++++++++++++----------------------- 1 file changed, 44 insertions(+), 36 deletions(-) diff --git a/format-diff.py b/format-diff.py index 1571d24744d..55df370e46c 100755 --- a/format-diff.py +++ b/format-diff.py @@ -1,44 +1,48 @@ -#!/usr/bin/python3 +# +# 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. import argparse import os import subprocess from os import path - import json - - def run_cmd(cmd, show_cmd=False): res = os.popen(cmd).readlines() if show_cmd: print("RUN CMD: {}".format(cmd)) return res - - def main(): default_suffix = ['.cpp', '.h', '.cc', '.hpp'] parser = argparse.ArgumentParser(description='TiFlash Code Format', formatter_class=argparse.ArgumentDefaultsHelpFormatter) - parser.add_argument('--repo_path', help='path of tics repository', + parser.add_argument('--repo_path', help='path of tiflash repository', default=os.path.dirname(os.path.abspath(__file__))) parser.add_argument('--suffix', help='suffix of files to format, split by space', default=' '.join(default_suffix)) - parser.add_argument('--ignore_suffix', help='ignore files with suffix, split by space') - parser.add_argument('--diff_from', help='commit hash/branch to check git diff', default='HEAD') - parser.add_argument('--check_formatted', help='exit -1 if NOT formatted', action='store_true') - parser.add_argument('--dump_diff_files_to', help='dump diff file names to specific path', default=None) - + parser.add_argument('--ignore_suffix', + help='ignore files with suffix, split by space') + parser.add_argument( + '--diff_from', help='commit hash/branch to check git diff', default='HEAD') + parser.add_argument('--check_formatted', + help='exit -1 if NOT formatted', action='store_true') + parser.add_argument('--dump_diff_files_to', + help='dump diff file names to specific path', default=None) args = parser.parse_args() default_suffix = args.suffix.strip().split(' ') if args.suffix else [] - ignore_suffix = args.ignore_suffix.strip().split(' ') if args.ignore_suffix else [] - tics_repo_path = args.repo_path - if not os.path.isabs(tics_repo_path): + ignore_suffix = args.ignore_suffix.strip().split( + ' ') if args.ignore_suffix else [] + tiflash_repo_path = args.repo_path + if not os.path.isabs(tiflash_repo_path): raise Exception("path of repo should be absolute") - assert tics_repo_path[-1] != '/' - - os.chdir(tics_repo_path) + assert tiflash_repo_path[-1] != '/' + os.chdir(tiflash_repo_path) files_to_check = run_cmd('git diff HEAD --name-only') if args.diff_from == 'HEAD' else run_cmd( 'git diff {} --name-only'.format(args.diff_from)) - files_to_check = [os.path.join(tics_repo_path, s.strip()) for s in files_to_check] + files_to_check = [os.path.join(tiflash_repo_path, s.strip()) + for s in files_to_check] files_to_format = [] for f in files_to_check: if not any([f.endswith(e) for e in default_suffix]): @@ -52,35 +56,39 @@ def main(): print('file {} can not be formatted'.format(file_path)) continue files_to_format.append(file_path) - if args.dump_diff_files_to: - da = [e[len(tics_repo_path):] for e in files_to_format] - json.dump({'files': da, 'repo': tics_repo_path}, open(args.dump_diff_files_to, 'w')) - print('dump {} modified files info to {}'.format(len(da), args.dump_diff_files_to)) - + da = [e[len(tiflash_repo_path):] for e in files_to_format] + json.dump({'files': da, 'repo': tiflash_repo_path}, + open(args.dump_diff_files_to, 'w')) + print('dump {} modified files info to {}'.format( + len(da), args.dump_diff_files_to)) if files_to_format: print('Files to format:\n {}'.format('\n '.join(files_to_format))) + for file in files_to_format: + cmd = 'clang-format -i {}'.format(file) + if subprocess.Popen(cmd, shell=True, cwd=tiflash_repo_path).wait(): + exit(-1) if args.check_formatted: - cmd = 'clang-format -i {}'.format(' '.join(files_to_format)) - if subprocess.Popen(cmd, shell=True, cwd=tics_repo_path).wait(): - exit(-1) diff_res = run_cmd('git diff --name-only') - if diff_res: + files_not_in_contrib = [f for f in diff_res if not f.startswith('contrib')] + files_contrib = [f for f in diff_res if f.startswith('contrib')] + if files_not_in_contrib: + print('') print('Error: found files NOT formatted') - print(''.join(diff_res)) - print(''.join(run_cmd('git diff'))) + print(''.join(files_not_in_contrib)) exit(-1) + elif files_contrib: + print('') + print('Warn: found contrib changed') + print(''.join(files_contrib)) + print('') + print(''.join(run_cmd('git status'))) else: print("Format check passed") else: - cmd = 'clang-format -i {}'.format(' '.join(files_to_format)) - if subprocess.Popen(cmd, shell=True, cwd=tics_repo_path).wait(): - exit(-1) print("Finish code format") else: print('No file to format') - - if __name__ == '__main__': - main() + main() \ No newline at end of file