From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 52CA72A798 for ; Thu, 18 Apr 2019 13:54:34 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id AgLq39ycUG9E for ; Thu, 18 Apr 2019 13:54:34 -0400 (EDT) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 0EAE42A791 for ; Thu, 18 Apr 2019 13:54:33 -0400 (EDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH 2/9] sql: disallow text values participate in sum() aggregate From: "n.pettik" In-Reply-To: <829e1d67-9335-65a0-714f-a5684dc3aab5@tarantool.org> Date: Thu, 18 Apr 2019 20:54:32 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <829e1d67-9335-65a0-714f-a5684dc3aab5@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > On 16 Apr 2019, at 17:12, Vladislav Shpilevoy = wrote: >=20 > Hi! Thanks for the patch! See 3 comments below. >=20 > On 14/04/2019 18:04, Nikita Pettik wrote: >> It is obvious that we can't add string with number except the case = when >> string is a number literal in quotes (aka '123' or '0.5'). Before = this >> patch values which can't be converted to numbers were just skipped. >> Now error is raised. Another one misbehavior was in using >> sql_value_numeric_type() function, which set flag indicating number >> value in memory cell, but didn't clear MEM_Str flag. >=20 > 1. Before this concrete commit sql_value_numeric_type() worked = correctly - > it used your refactored mem_apply_numeric_type(), which clears the old > type flags. Ok, indeed. Fixed commit message to the next: sql: disallow text values participate in sum() aggregate =20 It is obvious that we can't add string with number except the case = when string is a number literal in quotes (aka '123' or '0.5'). Before = this patch values which can't be converted to numbers were just skipped. Now error is raised. This patch also removes = sql_value_numeric_type() function since it is not used anymore: instead we invoke mem_apply_numeric_type(). >> As a result, we >> couldn't determine type of value in such memory cell without >> ambigiousness. >=20 > 2. Can't find this word in a dictionary. Probably, ambiguousness? Removed this mention at all. See above. >> index 8cd00d43a..ec9123a66 100644 >> --- a/src/box/sql/vdbeInt.h >> +++ b/src/box/sql/vdbeInt.h >> @@ -274,8 +274,7 @@ mem_type_to_str(const struct Mem *p); >> * if we can do so without loss of information. Firstly, value >> * is attempted to be converted to integer, and in case of fail - >> * to floating point number. Note that function is assumed to be >> - * called only on memory cell containing string, >> - * i.e. memory->type =3D=3D MEM_Str. >> + * called on memory cell containing string, i.e. mem->type =3D=3D = MEM_Str. >=20 > 3. Please, move to the previous commit. Moved.