From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 3F0246BD0D; Sun, 11 Apr 2021 21:16:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3F0246BD0D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618164994; bh=LlVXaZ6UfRPx479QH2j1DXuBP+hf8V5bwUw6zFFCkhI=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=LZBq9MbYHL9EirYeTESmsNC2nHp9HarTvIE6Qaii0rKKUz9xUGT/+vytlQ3/whq61 JX6vXoT04wUUwWp8R0Y/Z4J36wNtQ8kNNDIOqupi5Wa8uNKsgL5b7pd5TU4QOy9f3s 4Jq66WiNYSpo1WP9gCMA3QUkYZym0LAsHaPUB/2I= Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id DCB616BD0C for ; Sun, 11 Apr 2021 21:16:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DCB616BD0C Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lVedL-0006PG-Th; Sun, 11 Apr 2021 21:16:32 +0300 To: imeevma@tarantool.org, tsafin@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: <5a71515faac5f305fc3bd1ebbc20d1dd65bf027c.1617984948.git.imeevma@gmail.com> Message-ID: <85edf743-dd85-d4f7-2aad-05802ea67e9e@tarantool.org> Date: Sun, 11 Apr 2021 20:16:30 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <5a71515faac5f305fc3bd1ebbc20d1dd65bf027c.1617984948.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480B1C8842CE613979723F2FB4628545A35182A05F538085040CABA430BA82387F5427EF0A9E5F57A2869CC2D64287F3A19174C6D45BAFB66AD X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE731D82F3F177D3BCDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637FBDDB35DE4C816078638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B274DD66B39410A8A0B6C2E9948FE8C7EA89428004B1FE94FDD2E47CDBA5A96583C09775C1D3CA48CFE478A468B35FE767117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7ABB305BD10C6E5099FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735CE5475246E174218B5C8C57E37DE458BEDA766A37F9254B7 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C97831C8038E6165E86F0FB1D76F471CF06CBF X-C1DE0DAB: 0D63561A33F958A5ADF32919F4F4C7ECD3186C9524442AB205330CE1B434E848D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34023CA4E4726A7D6C4CC76E736C6F31F3560144491D321DA2BB0FAF855E9B53A48513F5AC15A3EA601D7E09C32AA3244CDCA880ABDC2763CE3BFE1E936B6A9C94C86C126E7119A0FEFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyKKJYJ15DtKI1RfVLk3qzA== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822F512B5C7F65388FD49C6C633C00EE1383841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 20/52] sql: introduce mem_compare() X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Nice fixes! This is the last email for today, I will continue the review of the patchset tomorrow. See 6 comments below. > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index 859e337aa..eee72a7fe 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -624,6 +624,211 @@ mem_rem(const struct Mem *left, const struct Mem *right, struct Mem *result) > return 0; > } > > +static int > +compare_blobs(const struct Mem *a, const struct Mem *b, int *result) > +{ 1. Would be good to have an assertion here that both types are MEM_Blob. > + int an = a->n; > + int bn = b->n; > + int minlen = MIN(an, bn); > + > + /* > + * It is possible to have a Blob value that has some non-zero content > + * followed by zero content. But that only comes up for Blobs formed > + * by the OP_MakeRecord opcode, and such Blobs never get passed into > + * mem_compare(). > + */ > + assert((a->flags & MEM_Zero) == 0 || an == 0); > + assert((b->flags & MEM_Zero) == 0 || bn == 0); > + > + if ((a->flags & b->flags & MEM_Zero) != 0) { > + *result = a->u.nZero - b->u.nZero; > + return 0; > + } > + if ((a->flags & MEM_Zero) != 0) { > + for (int i = 0; i < minlen; ++i) { > + if (b->z[i] != 0) { > + *result = -1; > + return 0; > + } > + } > + *result = a->u.nZero - bn; > + return 0; > + } > + if ((b->flags & MEM_Zero) != 0) { > + for (int i = 0; i < minlen; ++i) { > + if (a->z[i] != 0){ > + *result = 1; > + return 0; > + } > + } > + *result = b->u.nZero - an; > + return 0; > + } > + *result = memcmp(a->z, b->z, minlen); > + if (*result != 0) > + return 0; > + *result = an - bn; > + return 0; 2. compare_blobs never fails. So you can drop result out argument and return the comparison result as 'return'. > +} > + > +static int > +compare_numbers(const struct Mem *left, const struct Mem *right, int *result) > +{ > + struct sql_num a, b; > + /* TODO: Here should be check for right value type. */ 3. What if 'b' is a string, which can't be converted to a number? > + if (get_number(right, &b) != 0) { > + *result = -1; > + return 0; > + } > + if (get_number(left, &a) != 0) {> + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(left), > + "numeric"); > + return -1; > + } > + if (a.type == MEM_Real) { > + if (b.type == MEM_Real) { > + if (a.d > b.d) > + *result = 1; > + else if (a.d < b.d) > + *result = -1; > + else > + *result = 0; > + return 0; > + } > + if (b.type == MEM_Int) > + *result = double_compare_nint64(a.d, b.i, 1); > + else > + *result = double_compare_uint64(a.d, b.u, 1); > + return 0; > + } > + if (a.type == MEM_Int) { > + if (b.type == MEM_Int) { > + if (a.i > b.i) > + *result = 1; > + else if (a.i < b.i) > + *result = -1; > + else > + *result = 0; > + return 0; > + } > + if (b.type == MEM_UInt) > + *result = -1; > + else > + *result = double_compare_nint64(b.d, a.i, -1); > + return 0; > + } > + assert(a.type == MEM_UInt); > + if (b.type == MEM_UInt) { > + if (a.u > b.u) > + *result = 1; > + else if (a.u < b.u) > + *result = -1; > + else > + *result = 0; > + return 0; > + } > + if (b.type == MEM_Int) > + *result = 1; > + else > + *result = double_compare_uint64(b.d, a.u, -1); > + return 0; > +} > + > +static int > +compare_strings(const struct Mem *left, const struct Mem *right, int *result, > + const struct coll *coll) > +{ > + char *a; > + uint32_t an; > + char bufl[BUF_SIZE]; > + if ((left->flags & MEM_Str) != 0) { > + a = left->z; > + an = left->n; > + } else { > + assert((left->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0); > + a = &bufl[0]; > + if ((left->flags & MEM_Int) != 0) > + sql_snprintf(BUF_SIZE, a, "%lld", left->u.i); > + else if ((left->flags & MEM_UInt) != 0) > + sql_snprintf(BUF_SIZE, a, "%llu", left->u.u); > + else > + sql_snprintf(BUF_SIZE, a, "%!.15g", left->u.r); > + an = strlen(a); > + } > + > + char *b; > + uint32_t bn; > + char bufr[BUF_SIZE]; > + if ((right->flags & MEM_Str) != 0) { > + b = right->z; > + bn = right->n; > + } else { > + assert((right->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0); > + b = &bufr[0]; > + if ((right->flags & MEM_Int) != 0) > + sql_snprintf(BUF_SIZE, b, "%lld", right->u.i); > + else if ((right->flags & MEM_UInt) != 0) > + sql_snprintf(BUF_SIZE, b, "%llu", right->u.u); > + else > + sql_snprintf(BUF_SIZE, b, "%!.15g", right->u.r); > + bn = strlen(b); > + } > + if (coll) { > + *result = coll->cmp(a, an, b, bn, coll); > + return 0; > + } > + uint32_t minlen = MIN(an, bn); > + *result = memcmp(a, b, minlen); > + if (*result != 0) > + return 0; > + *result = an - bn; > + return 0; 4. It can't fail either. You can return result as 'return'. > +} > + > +int > +mem_compare(const struct Mem *left, const struct Mem *right, int *result, > + enum field_type type, struct coll *coll) > +{ > + assert(((left->flags | right->flags) & MEM_Null) == 0); > + int flags_any = left->flags | right->flags; 5. 'any' isn't needed until the next branch. You can move it right above if ((flags_any & MEM_Bool) != 0) { > + int flags_all = left->flags & right->flags; > + > + if ((flags_all & MEM_Bool) != 0) { > + if (left->u.b == right->u.b) > + *result = 0; > + else if (left->u.b) > + *result = 1; > + else > + *result = -1; > + return 0; > + } > + if ((flags_any & MEM_Bool) != 0) { > + char *str = (left->flags & MEM_Bool) == 0 ? > + mem_type_to_str(left) : mem_type_to_str(right); > + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str, "boolean"); > + return -1; > + } > + > + if ((flags_all & MEM_Blob) != 0) > + return compare_blobs(left, right, result); > + if ((flags_any & MEM_Blob) != 0) { > + char *str = (left->flags & MEM_Blob) == 0 ? > + mem_type_to_str(left) : mem_type_to_str(right); > + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str, "varbinary"); > + return -1; > + } > + > + if (type == FIELD_TYPE_STRING) > + return compare_strings(left, right, result, coll); > + > + if (sql_type_is_numeric(type) || > + (flags_any & (MEM_Int | MEM_UInt | MEM_Real)) != 0) > + return compare_numbers(left, right, result); > + > + assert((left->flags & MEM_Str) != 0 && (right->flags & MEM_Str) != 0); > + return compare_strings(left, right, result, coll); > +} > diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h > index 69a7d9f7a..6c022d8d8 100644 > --- a/src/box/sql/mem.h > +++ b/src/box/sql/mem.h > @@ -226,6 +226,11 @@ mem_div(const struct Mem *left, const struct Mem *right, struct Mem *result); > int > mem_rem(const struct Mem *left, const struct Mem *right, struct Mem *result); > > +/** Compare two non-NULL MEMs and return the result of comparison. */ > +int > +mem_compare(const struct Mem *left, const struct Mem *right, int *result, > + enum field_type type, struct coll *coll); 6. What is 'type'? Can you keep it out of the comparator somehow? For example, make it 3 functions: mem_cmp_as_str, mem_cmp_as_num, and just a generic mem_cmp without any types calling the first 2 comparators. Otherwise the type thing is super ugly. It leaks the details of opcodes into mem.c.