Tarantool development patches archive
 help / color / mirror / Atom feed
From: Stanislav Zudin <szudin@tarantool.org>
To: tarantool-patches@freelists.org, "n.pettik" <korablev@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers
Date: Mon, 1 Apr 2019 23:43:27 +0300	[thread overview]
Message-ID: <071a569b-de79-1afe-7335-44de04d6a867@tarantool.org> (raw)
In-Reply-To: <A2089B11-9879-4DEB-9A7A-CBB657211CA5@tarantool.org>



On 25.03.2019 18:13, n.pettik wrote:
> 
>> 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(-)
>>
>> 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’ve 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, …);

Done.

> 
> 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.

It's a bad practice to write diagnostic from the low-level functions.
Using a single enum as a return value gives a compact readable code.

> 
>> +
>> 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)
>> }
>>
>> +/*
>> + * 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 = *pA;
>> -	testcase(iA == 0);
>> -	testcase(iA == 1);
>> -	testcase(iB == -1);
>> -	testcase(iB == 0);
>> -	if (iB >= 0) {
>> -		testcase(iA > 0 && LARGEST_INT64 - iA == iB);
>> -		testcase(iA > 0 && LARGEST_INT64 - iA == iB - 1);
>> -		if (iA > 0 && LARGEST_INT64 - iA < iB)
>> -			return 1;
>> +
>> +	bool is_negA = iA < 0 && is_signedA;
>> +	bool is_negB = 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.
>   */
> 

Fixed.

>> /*
>> 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)!=0) {
>> 		iA = pIn1->u.i;
>> 		iB = pIn2->u.i;
>> +		bool is_signedA = (type1 & MEM_Unsigned) == 0;
>> +		bool is_signedB = (type2 & MEM_Unsigned) == 0;
>> 		bIntint = 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 == 0)
>> -				goto division_by_zero;
>> -			if (iA==-1 && iB==SMALLEST_INT64) goto integer_overflow;
>> -			iB /= iA;
>> -			break;
>> +		case OP_Add:       arr = sqlAddInt64(&iB, is_signedA, iA, is_signedB); break;
>> +		case OP_Subtract:  arr = sqlSubInt64(&iB, is_signedA, iA, is_signedB); break;
>> +		case OP_Multiply:  arr = sqlMulInt64(&iB, is_signedA, iA, is_signedB); break;
>> +		case OP_Divide:    arr = sqlDivInt64(&iB, is_signedA, iA, is_signedB); break;
>> +		default: 	   arr = sqlRemInt64(&iB, is_signedA, iA, is_signedB); break;
> 
> SQL ANSI specifications doesn’t provide description of unsigned behaviour.
> But for example in C there is no unsigned overflow, because if result can’t
> be represented by unsigned range, it is truncated to modulo (MAX_UINT + 1 == 1).
> Should we follow this way? IDK, it needs discussion involving other team members.

Good point.

> 
> 

  reply	other threads:[~2019-04-01 20:43 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
2019-03-15 15:45 ` [PATCH 01/13] sql: Convert big integers from string Stanislav Zudin
2019-03-25 15:10   ` [tarantool-patches] " n.pettik
2019-04-01 20:39     ` Stanislav Zudin
2019-04-02  7:27     ` Konstantin Osipov
2019-03-15 15:45 ` [PATCH 02/13] sql: make VDBE recognize big integers Stanislav Zudin
2019-03-25 15:11   ` [tarantool-patches] " n.pettik
2019-04-01 20:42     ` Stanislav Zudin
2019-04-02  7:38   ` [tarantool-patches] " Konstantin Osipov
2019-03-15 15:45 ` [PATCH 03/13] sql: removes unused function Stanislav Zudin
2019-03-25 15:11   ` [tarantool-patches] " n.pettik
2019-04-01 20:39     ` Stanislav Zudin
2019-03-15 15:45 ` [PATCH 04/13] sql: support big integers within sql binding Stanislav Zudin
2019-03-25 15:12   ` [tarantool-patches] " n.pettik
2019-04-01 20:42     ` Stanislav Zudin
2019-04-02  7:46     ` Konstantin Osipov
2019-04-02  7:44   ` [tarantool-patches] " Konstantin Osipov
2019-03-15 15:45 ` [PATCH 05/13] sql: removes redundant function Stanislav Zudin
2019-03-25 15:12   ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 06/13] sql: aux functions to support big integers Stanislav Zudin
2019-03-25 15:13   ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 07/13] sql: arithmetic functions " Stanislav Zudin
2019-03-25 15:13   ` [tarantool-patches] " n.pettik
2019-04-01 20:43     ` Stanislav Zudin [this message]
2019-04-02  7:54       ` Konstantin Osipov
2019-04-02  7:52     ` Konstantin Osipov
2019-03-15 15:45 ` [PATCH 08/13] sql: aggregate sql functions support big int Stanislav Zudin
2019-03-25 15:13   ` [tarantool-patches] " n.pettik
2019-04-01 20:43     ` Stanislav Zudin
2019-04-02  7:57   ` [tarantool-patches] " Konstantin Osipov
2019-03-15 15:45 ` [PATCH 09/13] sql: fixes errors Stanislav Zudin
2019-03-25 15:14   ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 10/13] sql: fixes an error in sqlSubInt64 Stanislav Zudin
2019-03-25 15:14   ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 11/13] sql: fixes an error in string to int64 conversion Stanislav Zudin
2019-03-25 15:14   ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 12/13] sql: fixes an error in uint64 to double casting Stanislav Zudin
2019-03-25 15:15   ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 13/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
2019-03-25 15:25   ` [tarantool-patches] " n.pettik
2019-04-01 20:44     ` Stanislav Zudin
2019-03-25 15:10 ` [tarantool-patches] Re: [PATCH 00/13] " n.pettik
2019-04-01 20:38   ` Stanislav Zudin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=071a569b-de79-1afe-7335-44de04d6a867@tarantool.org \
    --to=szudin@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox