From 9de2e01b649cd964d5313f2a581bfdc307f4dc60 Mon Sep 17 00:00:00 2001 From: guo-shaoge Date: Wed, 13 Nov 2024 17:42:21 +0800 Subject: [PATCH 1/5] fix lpad/rpad trailing zero miss Signed-off-by: guo-shaoge --- dbms/src/Functions/FunctionsString.cpp | 20 ++++++++++++-------- tests/fullstack-test/expr/pad.test | 15 +++++++++++++++ 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/dbms/src/Functions/FunctionsString.cpp b/dbms/src/Functions/FunctionsString.cpp index b042ec98e5f..df76c7af47c 100644 --- a/dbms/src/Functions/FunctionsString.cpp +++ b/dbms/src/Functions/FunctionsString.cpp @@ -3622,6 +3622,14 @@ class TidbPadImpl } } + static void addTrailingZero(ColumnString::Chars_t & res, + ColumnString::Offset & res_offset) + { + res.resize(res.size() + 1); + res[res_offset] = '\0'; + res_offset++; + } + // Do padding for one row. // data_size/padding_size includes the tailing '\0'. // Return true if result is null. @@ -3640,6 +3648,7 @@ class TidbPadImpl if (target_len < 0 || (data_len < static_cast(target_len) && pad_len == 0)) { + addTrailingZero(res, res_offset); return true; } @@ -3691,10 +3700,7 @@ class TidbPadImpl ++left; } } - // Add trailing zero. - res.resize(res.size() + 1); - res[res_offset] = '\0'; - res_offset++; + addTrailingZero(res, res_offset); return false; } @@ -3714,6 +3720,7 @@ class TidbPadImpl if (target_len < 0 || (data_len < static_cast(target_len) && pad_len == 0)) { + addTrailingZero(res, res_offset); return true; } @@ -3766,10 +3773,7 @@ class TidbPadImpl copyResult(res, res_offset, data, 0, tmp_target_len); res_offset += tmp_target_len; } - // Add trailing zero. - res.resize(res.size() + 1); - res[res_offset] = '\0'; - res_offset++; + addTrailingZero(res, res_offset); return false; } diff --git a/tests/fullstack-test/expr/pad.test b/tests/fullstack-test/expr/pad.test index 890eae62e91..2aa9df31ecd 100644 --- a/tests/fullstack-test/expr/pad.test +++ b/tests/fullstack-test/expr/pad.test @@ -111,3 +111,18 @@ mysql> set tidb_isolation_read_engines='tiflash'; set tidb_enforce_mpp=1; select mysql> set tidb_isolation_read_engines='tiflash'; set tidb_enforce_mpp=1; SELECT max(lpad('y',0,c1)) FROM test.t2 max(lpad('y',0,c1)) + + +mysql> drop table if exists test.t1 +mysql> create table test.t1(c1 varchar(100), c2 int) +mysql> alter table test.t1 set tiflash replica 1 +mysql> insert into test.t1 values('a', -1) +func> wait_table test t1 + +# crc32 will call ColumnString::getDataAt(i), which assume all string rows ends with '\0'. +mysql> select crc32(lpad(c1, c2, 'b')) from t1 ++--------------------------+ +| crc32(lpad(c1, c2, 'b')) | ++--------------------------+ +| NULL | ++--------------------------+ From 081dc30be77b7f95c80070d9192f38de4e05c2e2 Mon Sep 17 00:00:00 2001 From: guo-shaoge Date: Wed, 13 Nov 2024 17:46:13 +0800 Subject: [PATCH 2/5] fix Signed-off-by: guo-shaoge --- dbms/src/Functions/FunctionsString.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dbms/src/Functions/FunctionsString.cpp b/dbms/src/Functions/FunctionsString.cpp index df76c7af47c..6119c7be124 100644 --- a/dbms/src/Functions/FunctionsString.cpp +++ b/dbms/src/Functions/FunctionsString.cpp @@ -3622,8 +3622,7 @@ class TidbPadImpl } } - static void addTrailingZero(ColumnString::Chars_t & res, - ColumnString::Offset & res_offset) + static void addTrailingZero(ColumnString::Chars_t & res, ColumnString::Offset & res_offset) { res.resize(res.size() + 1); res[res_offset] = '\0'; From 2380749a45f895ea55f1e5bb4b2c92ac5841df59 Mon Sep 17 00:00:00 2001 From: guo-shaoge Date: Wed, 13 Nov 2024 17:49:05 +0800 Subject: [PATCH 3/5] fix Signed-off-by: guo-shaoge --- tests/fullstack-test/expr/pad.test | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/fullstack-test/expr/pad.test b/tests/fullstack-test/expr/pad.test index 2aa9df31ecd..e68c97c8b7c 100644 --- a/tests/fullstack-test/expr/pad.test +++ b/tests/fullstack-test/expr/pad.test @@ -120,9 +120,10 @@ mysql> insert into test.t1 values('a', -1) func> wait_table test t1 # crc32 will call ColumnString::getDataAt(i), which assume all string rows ends with '\0'. -mysql> select crc32(lpad(c1, c2, 'b')) from t1 +mysql> select crc32(lpad(c1, c2, 'b')) from test.t1 +--------------------------+ | crc32(lpad(c1, c2, 'b')) | +--------------------------+ | NULL | +--------------------------+ +mysql> drop table if exists test.t1 From 8322b9605df1a40ffbaf99abff68052e9b337e4b Mon Sep 17 00:00:00 2001 From: guo-shaoge Date: Thu, 14 Nov 2024 10:02:53 +0800 Subject: [PATCH 4/5] fix Signed-off-by: guo-shaoge --- dbms/src/Functions/FunctionsString.cpp | 2 +- tests/fullstack-test/expr/pad.test | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/dbms/src/Functions/FunctionsString.cpp b/dbms/src/Functions/FunctionsString.cpp index 6119c7be124..7eeabfca452 100644 --- a/dbms/src/Functions/FunctionsString.cpp +++ b/dbms/src/Functions/FunctionsString.cpp @@ -3626,7 +3626,7 @@ class TidbPadImpl { res.resize(res.size() + 1); res[res_offset] = '\0'; - res_offset++; + ++res_offset; } // Do padding for one row. diff --git a/tests/fullstack-test/expr/pad.test b/tests/fullstack-test/expr/pad.test index e68c97c8b7c..40382763995 100644 --- a/tests/fullstack-test/expr/pad.test +++ b/tests/fullstack-test/expr/pad.test @@ -126,4 +126,12 @@ mysql> select crc32(lpad(c1, c2, 'b')) from test.t1 +--------------------------+ | NULL | +--------------------------+ + +mysql> select crc32(rpad(c1, c2, 'b')) from test.t1 ++--------------------------+ +| crc32(rpad(c1, c2, 'b')) | ++--------------------------+ +| NULL | ++--------------------------+ + mysql> drop table if exists test.t1 From b2133712f32441510a5f4ebc6e91f5f6d04adc20 Mon Sep 17 00:00:00 2001 From: guo-shaoge Date: Thu, 14 Nov 2024 20:13:54 +0800 Subject: [PATCH 5/5] refine Signed-off-by: guo-shaoge --- dbms/src/Functions/FunctionsString.cpp | 30 ++++++++++---------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/dbms/src/Functions/FunctionsString.cpp b/dbms/src/Functions/FunctionsString.cpp index 7eeabfca452..bfb057f0f60 100644 --- a/dbms/src/Functions/FunctionsString.cpp +++ b/dbms/src/Functions/FunctionsString.cpp @@ -3146,6 +3146,13 @@ class FunctionTiDBTrim : public IFunction class TidbPadImpl { + static void addTrailingZero(ColumnString::Chars_t & res, ColumnString::Offset & res_offset) + { + res.resize(res.size() + 1); + res[res_offset] = '\0'; + ++res_offset; + } + public: template static void tidbExecutePadImpl( @@ -3435,9 +3442,7 @@ class TidbPadImpl } else { - result_data.resize(result_data.size() + 1); - result_data[res_prev_offset] = '\0'; - res_prev_offset++; + addTrailingZero(result_data, res_prev_offset); } string_prev_offset = string_offsets[i]; @@ -3496,9 +3501,7 @@ class TidbPadImpl } else { - result_data.resize(result_data.size() + 1); - result_data[res_prev_offset] = '\0'; - res_prev_offset++; + addTrailingZero(result_data, res_prev_offset); } string_prev_offset = string_offsets[i]; @@ -3555,9 +3558,7 @@ class TidbPadImpl } else { - result_data.resize(result_data.size() + 1); - result_data[res_prev_offset] = '\0'; - res_prev_offset++; + addTrailingZero(result_data, res_prev_offset); } padding_prev_offset = (*padding_offsets)[i]; @@ -3613,22 +3614,13 @@ class TidbPadImpl } else { - result_data.resize(result_data.size() + 1); - result_data[res_prev_offset] = '\0'; - res_prev_offset++; + addTrailingZero(result_data, res_prev_offset); } result_offsets[i] = res_prev_offset; } } - static void addTrailingZero(ColumnString::Chars_t & res, ColumnString::Offset & res_offset) - { - res.resize(res.size() + 1); - res[res_offset] = '\0'; - ++res_offset; - } - // Do padding for one row. // data_size/padding_size includes the tailing '\0'. // Return true if result is null.