[Tarantool-patches] [PATCH v1 16/21] sql: refactor REPLACE() function
Mergen Imeev
imeevma at tarantool.org
Mon Oct 25 11:45:58 MSK 2021
Thank you for the review! My answer and diff below.
On Fri, Oct 15, 2021 at 12:45:29AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> On 08.10.2021 19:32, Mergen Imeev via Tarantool-patches wrote:
> > Part of #4145
> > ---
> > src/box/sql/func.c | 58 ++++++++++++++++------------------------------
> > 1 file changed, 20 insertions(+), 38 deletions(-)
> >
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index a3ca53545..f26d101e9 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -1460,34 +1460,26 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
> > int i, j; /* Loop counters */
> >
> > assert(argc == 3);
> > - UNUSED_PARAMETER(argc);
> > - zStr = mem_as_ustr(&argv[0]);
> > - if (zStr == 0)
> > - return;
> > - nStr = mem_len_unsafe(&argv[0]);
> > - assert(zStr == mem_as_ustr(&argv[0])); /* No encoding change */
> > - zPattern = mem_as_ustr(&argv[1]);
> > - if (zPattern == 0) {
> > - assert(mem_is_null(&argv[1])
> > - || sql_context_db_handle(context)->mallocFailed);
> > + (void)argc;
> > + if (mem_is_any_null(&argv[0], &argv[1]) || mem_is_null(&argv[2]))
> > return;
> > - }
> > - nPattern = mem_len_unsafe(&argv[1]);
> > + assert(mem_is_bytes);
>
> 1. You forgot to call the function.
>
Thanks, fixed.
> > + zStr = (const unsigned char *)argv[0].z;
> > + nStr = argv[0].n;
> > + zPattern = (const unsigned char *)argv[1].z;
> > + nPattern = argv[1].n;
> > if (nPattern == 0) {
> > - assert(!mem_is_null(&argv[1]));
> > - sql_result_value(context, &argv[0]);
> > + if (mem_copy(context->pOut, &argv[0]) != 0)
> > + context->is_aborted = true;
> > return;
> > @@ -1497,22 +1489,12 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
> > zOut[j++] = zStr[i];
> > } else {
> > u8 *zOld;
> > - sql *db = sql_context_db_handle(context);
> > nOut += nRep - nPattern;
> > - testcase(nOut - 1 == db->aLimit[SQL_LIMIT_LENGTH]);
> > - testcase(nOut - 2 == db->aLimit[SQL_LIMIT_LENGTH]);
> > - if (nOut - 1 > db->aLimit[SQL_LIMIT_LENGTH]) {
>
> 2. Why did you drop length limit assertions and checks?
>
These assertions do nothing, since testcase macro defined this way:
#define testcase(X)
I don't think we should check the length of the STRING and VARBINARY values in
functions, since the result is checked in the VDBE.
> > - diag_set(ClientError, ER_SQL_EXECUTE, "string "\
> > - "or binary string is too big");
> > - context->is_aborted = true;
> > - sql_free(zOut);
> > - return;
> > - }
Diff:
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 91ff741df..f7024a9b9 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1485,7 +1485,8 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
(void)argc;
if (mem_is_any_null(&argv[0], &argv[1]) || mem_is_null(&argv[2]))
return;
- assert(mem_is_bytes);
+ assert(mem_is_bytes(&argv[0]) && mem_is_bytes(&argv[1]) &&
+ mem_is_bytes(&argv[2]));
zStr = (const unsigned char *)argv[0].z;
nStr = argv[0].n;
zPattern = (const unsigned char *)argv[1].z;
More information about the Tarantool-patches
mailing list