[tarantool-patches] Re: [PATCH] sql: fix bug with BLOB TRIM() when X'00' in char set
Roman
roman.habibov at tarantool.org
Tue Dec 18 17:30:36 MSK 2018
Hi! Thanks for review.
> Nit: ‘… after its’ -> ‘… after it’.
>
> Nit: inculding -> including.
>
> Nit: ‘are then ignored’ -> ‘are ignored’.
>
> Nit: I wouldn’t mix first and second conditions:
> ‘If the char set contains …, all characters are ignored...'
Fixed.
> You should put links to the branch and issue below delimiter ---, not above.
Sorry.
> Lets start from putting here comments which explain
> what is going on here (how you algorithm/approach works).
> Then, lets choose more suitable names for variables:
> length, sizeInChar and nChar sound very similar
> (if you can’t come up with better names, then at least
> mention in comment what they mean).
>
>> + int sizeInChar = sqlite3_value_bytes(argv[1]);
>> + int length = 0;
> What is the difference between sizeInChar, length and nChar?
I add comments and renamed vars. Hope, now it's clearer.
> We use explicit conditions as a predicate value. In other words:
>
> while (sizeInChar - length > 0)
>
> Also, it would be better if you declare and initialise var at once:
>
> const unsigned char *z = zCharSet;
Fixed.
>> + zStepBack = z;
>> SQLITE_SKIP_UTF8(z);
>> + length += z - zStepBack;
>> + nChar++;
>> }
>> if (nChar > 0) {
>> azChar =
>> @@ -1235,10 +1243,16 @@ trimFunc(sqlite3_context * context, int argc, sqlite3_value ** argv)
>> return;
>> }
>> aLen = (unsigned char *)&azChar[nChar];
>> - for (z = zCharSet, nChar = 0; *z; nChar++) {
>> + z = zCharSet;
>> + nChar = 0;
>> + length = 0;
>> + while(sizeInChar - length) {
>> azChar[nChar] = (unsigned char *)z;
>> + zStepBack = z;
>> SQLITE_SKIP_UTF8(z);
>> + length += z - zStepBack;
>> aLen[nChar] = (u8) (z - azChar[nChar]);
>> + nChar++;
>> }
>> }
>> }
>> diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
>> index 393212968..b7b8e7c4c 100755
>> --- a/test/sql-tap/func.test.lua
>> +++ b/test/sql-tap/func.test.lua
>> @@ -1,6 +1,6 @@
>> #!/usr/bin/env tarantool
>> test = require("sqltester")
>> -test:plan(14535)
>> +test:plan(14547)
>>
>> --!./tcltestrunner.lua
>> -- 2001 September 15
>> @@ -2100,11 +2100,133 @@ test:do_execsql_test(
>> -- </func-22.22>
>> })
>>
>> -- This is to test the deprecated sqlite3_aggregate_count() API.
>> --
>> --test:do_test(
>> -- "func-23.1",
>> --- function()
>> +-- function()S
> Garbage diff.
Sorry. I's an accident.
commit 39b2481341cd2e71d67a04bec9aeed0e1189740c
Author: Roman Khabibov <roman.habibov at tarantool.org>
Date: Sat Dec 15 13:21:59 2018 +0300
sql: fix bug with BLOB TRIM() when X'00' in char set
The reason for the bug was that X'00' is a terminal symbol. If the
char set
contained X'00', all characters are ignored after it (including
itself).
Closes #3543
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 9667aead5..f397e23c1 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1223,9 +1223,19 @@ trimFunc(sqlite3_context * context, int argc,
sqlite3_value ** argv)
} else if ((zCharSet = sqlite3_value_text(argv[1])) == 0) {
return;
} else {
- const unsigned char *z;
- for (z = zCharSet, nChar = 0; *z; nChar++) {
+ const unsigned char *z = zCharSet;
+ int sizeOfCharSet = \
+ sqlite3_value_bytes(argv[1]); /* Size of char set in bytes. */
+ int nProcessedBytes = 0;
+ nChar = 0;
+ const unsigned char *zStepBack;
+ /* Count the number of UTF-8 characters passing through the
+ * entire char set, but not up to the '\0' or X'00' character. */
+ while(sizeOfCharSet - nProcessedBytes > 0) {
+ zStepBack = z;
SQLITE_SKIP_UTF8(z);
+ nProcessedBytes += z - zStepBack;
+ nChar++;
}
if (nChar > 0) {
azChar =
@@ -1235,10 +1245,18 @@ trimFunc(sqlite3_context * context, int argc,
sqlite3_value ** argv)
return;
}
aLen = (unsigned char *)&azChar[nChar];
- for (z = zCharSet, nChar = 0; *z; nChar++) {
+ z = zCharSet;
+ nChar = 0;
+ nProcessedBytes = 0;
+ /* Similar to the previous cycle. But
+ * now write into "azCharSet". */
+ while(sizeOfCharSet - nProcessedBytes > 0) {
azChar[nChar] = (unsigned char *)z;
+ zStepBack = z;
SQLITE_SKIP_UTF8(z);
+ nProcessedBytes += z - zStepBack;
aLen[nChar] = (u8) (z - azChar[nChar]);
+ nChar++;
}
}
}
diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index 393212968..b7de1d955 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(14535)
+test:plan(14547)
--!./tcltestrunner.lua
-- 2001 September 15
@@ -2100,6 +2100,128 @@ test:do_execsql_test(
-- </func-22.22>
})
+-- gh-3543 Check trimming of binary string when X'00' in trimming char set.
+
+test:do_execsql_test(
+ "func-22.23",
+ [[
+ SELECT TRIM(X'004100', X'00');
+ ]], {
+ -- <func-22.23>
+ "A"
+ -- </func-22.23>
+ })
+
+test:do_execsql_test(
+ "func-22.24",
+ [[
+ SELECT TRIM(X'004100', X'0000');
+ ]], {
+ -- <func-22.24>
+ "A"
+ -- </func-22.24>
+ })
+
+test:do_execsql_test(
+ "func-22.25",
+ [[
+ SELECT TRIM(X'004100', X'0042');
+ ]], {
+ -- <func-22.25>
+ "A"
+ -- </func-22.25>
+ })
+
+test:do_execsql_test(
+ "func-22.26",
+ [[
+ SELECT TRIM(X'00004100420000', X'00');
+ ]], {
+ -- <func-22.26>
+ "A\0B"
+ -- </func-22.26>
+ })
+
+test:do_execsql_test(
+ "func-22.27",
+ [[
+ SELECT LTRIM(X'004100', X'00');
+ ]], {
+ -- <func-22.27>
+ "A\0"
+ -- </func-22.27>
+ })
+
+test:do_execsql_test(
+ "func-22.28",
+ [[
+ SELECT LTRIM(X'004100', X'0000');
+ ]], {
+ -- <func-22.28>
+ "A\0"
+ -- </func-22.28>
+ })
+
+test:do_execsql_test(
+ "func-22.29",
+ [[
+ SELECT LTRIM(X'004100', X'0042');
+ ]], {
+ -- <func-22.29>
+ "A\0"
+ -- </func-22.29>
+ })
+
+test:do_execsql_test(
+ "func-22.30",
+ [[
+ SELECT LTRIM(X'00004100420000', X'00');
+ ]], {
+ -- <func-22.30>
+ "A\0B\0\0"
+ -- </func-22.30>
+ })
+
+test:do_execsql_test(
+ "func-22.31",
+ [[
+ SELECT RTRIM(X'004100', X'00');
+ ]], {
+ -- <func-22.31>
+ "\0A"
+ -- </func-22.31>
+ })
+
+test:do_execsql_test(
+ "func-22.32",
+ [[
+ SELECT RTRIM(X'004100', X'0000');
+ ]], {
+ -- <func-22.32>
+ "\0A"
+ -- </func-22.32>
+ })
+
+test:do_execsql_test(
+ "func-22.33",
+ [[
+ SELECT RTRIM(X'004100', X'0042');
+ ]], {
+ -- <func-22.33>
+ "\0A"
+ -- </func-22.33>
+ })
+
+test:do_execsql_test(
+ "func-22.34",
+ [[
+ SELECT RTRIM(X'00004100420000', X'00');
+ ]], {
+ -- <func-22.34>
+ "\0\0A\0B"
+ -- </func-22.34>
+ })
+
-- This is to test the deprecated sqlite3_aggregate_count() API.
--
--test:do_test(
More information about the Tarantool-patches
mailing list