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 7A14529C44 for ; Mon, 25 Mar 2019 11:13:16 -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 vEknpXkkQu7Y for ; Mon, 25 Mar 2019 11:13:16 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 3142C29C33 for ; Mon, 25 Mar 2019 11:13:16 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers From: "n.pettik" In-Reply-To: Date: Mon, 25 Mar 2019 18:13:13 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: 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: szudin@tarantool.org > Makes arithmetic functions accept arguments with > values in the range [2^63, 2^64). > --- > src/box/sql/func.c | 2 +- > src/box/sql/sqlInt.h | 23 +++- > src/box/sql/util.c | 236 ++++++++++++++++++++++++++++++++---------- > src/box/sql/vdbe.c | 36 ++++--- > src/box/sql/vdbeInt.h | 2 +- > 5 files changed, 223 insertions(+), 76 deletions(-) >=20 > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 9b1d7df9a..7f8e3f04e 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -4383,9 +4383,26 @@ Expr *sqlExprAddCollateString(Parse *, Expr *, = const char *); > Expr *sqlExprSkipCollate(Expr *); > int sqlCheckIdentifierName(Parse *, char *); > void sqlVdbeSetChanges(sql *, int); > -int sqlAddInt64(i64 *, i64); > -int sqlSubInt64(i64 *, i64); > -int sqlMulInt64(i64 *, i64); > + > +enum arithmetic_result { > + /* The result fits the signed 64-bit integer */ > + ATHR_SIGNED, > + /* The result is positive and fits the > + * unsigned 64-bit integer > + */ > + ATHR_UNSIGNED, > + /* The operation causes an overflow */ > + ATHR_OVERFLOW, > + /* The operation causes division by zero */ > + ATHR_DIVBYZERO > +}; > + > +enum arithmetic_result sqlAddInt64(i64 *, bool, i64, bool); > +enum arithmetic_result sqlSubInt64(i64 *, bool, i64, bool); > +enum arithmetic_result sqlMulInt64(i64 *, bool, i64, bool); > +enum arithmetic_result sqlDivInt64(i64 *, bool, i64, bool); > +enum arithmetic_result sqlRemInt64(i64 *, bool, i64, bool); Since you=E2=80=99ve already fixed signature of these functions, please make them follow Tarantool code style: enum arithmetic_result sql_add_int64(int64_t *lhs, bool is_lhs_signed, =E2=80=A6); What is more, personally I would apply the same fix as for atoi = functions: make them return -1 in case of overflow or division by 0 and set diag message right in these functions; use enum to represent their args. > + > int sqlAbsInt32(int); > #ifdef SQL_ENABLE_8_3_NAMES > void sqlFileSuffix3(const char *, char *); > diff --git a/src/box/sql/util.c b/src/box/sql/util.c > index be77f72f8..3786c5083 100644 > --- a/src/box/sql/util.c > +++ b/src/box/sql/util.c > @@ -1249,74 +1249,202 @@ sqlSafetyCheckSickOrOk(sql * db) > } >=20 > +/* > + * Attempt to add, substract, or multiply the 64-bit value iB against > + * the other 64-bit integer at *pA and store the result in *pA. > + * Return ATHR_SIGNED or ATHR_UNSIGNED on success. > + * Or if the operation would have resulted in an > + * overflow, leave *pA unchanged and return ATHR_OVERFLOW. > + */ > +enum arithmetic_result > +sqlAddInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB) > { > i64 iA =3D *pA; > - testcase(iA =3D=3D 0); > - testcase(iA =3D=3D 1); > - testcase(iB =3D=3D -1); > - testcase(iB =3D=3D 0); > - if (iB >=3D 0) { > - testcase(iA > 0 && LARGEST_INT64 - iA =3D=3D iB); > - testcase(iA > 0 && LARGEST_INT64 - iA =3D=3D iB - 1); > - if (iA > 0 && LARGEST_INT64 - iA < iB) > - return 1; > + > + bool is_negA =3D iA < 0 && is_signedA; > + bool is_negB =3D iB < 0 && is_signedB; > + > + /* Make sure we've got only one combination of > + * positive and negative operands > + */ Nit: note that correct way of comment formatting is: /* * Make sure we've got only one combination of * positive and negative operands. */ > /* > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index ea9d9d98f..d4bd845fb 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -1672,28 +1672,29 @@ case OP_Remainder: { /* same as = TK_REM, in1, in2, out3 */ > if ((type1 & type2 & MEM_Int)!=3D0) { > iA =3D pIn1->u.i; > iB =3D pIn2->u.i; > + bool is_signedA =3D (type1 & MEM_Unsigned) =3D=3D 0; > + bool is_signedB =3D (type2 & MEM_Unsigned) =3D=3D 0; > bIntint =3D 1; > + enum arithmetic_result arr; > switch( pOp->opcode) { > - case OP_Add: if (sqlAddInt64(&iB,iA)) goto = integer_overflow; break; > - case OP_Subtract: if (sqlSubInt64(&iB,iA)) goto = integer_overflow; break; > - case OP_Multiply: if (sqlMulInt64(&iB,iA)) goto = integer_overflow; break; > - case OP_Divide: { > - if (iA =3D=3D 0) > - goto division_by_zero; > - if (iA=3D=3D-1 && iB=3D=3DSMALLEST_INT64) goto = integer_overflow; > - iB /=3D iA; > - break; > + case OP_Add: arr =3D sqlAddInt64(&iB, is_signedA, = iA, is_signedB); break; > + case OP_Subtract: arr =3D sqlSubInt64(&iB, is_signedA, = iA, is_signedB); break; > + case OP_Multiply: arr =3D sqlMulInt64(&iB, is_signedA, = iA, is_signedB); break; > + case OP_Divide: arr =3D sqlDivInt64(&iB, is_signedA, = iA, is_signedB); break; > + default: arr =3D sqlRemInt64(&iB, is_signedA, = iA, is_signedB); break; SQL ANSI specifications doesn=E2=80=99t provide description of unsigned = behaviour. But for example in C there is no unsigned overflow, because if result = can=E2=80=99t be represented by unsigned range, it is truncated to modulo (MAX_UINT + = 1 =3D=3D 1). Should we follow this way? IDK, it needs discussion involving other team = members.