From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: "n.pettik" <korablev@tarantool.org>, tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints
Date: Thu, 11 Jul 2019 00:49:21 +0200 [thread overview]
Message-ID: <ba4b1598-867f-40f2-63c6-f16235043c55@tarantool.org> (raw)
In-Reply-To: <7AA862F1-50A9-40E4-A054-D42836D125A5@tarantool.org>
Hi! Thanks for the fixes!
On 05/07/2019 18:36, n.pettik wrote:
>
>
>> On 2 Jul 2019, at 00:53, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Thanks for the fixes!
>>
>>>>> @@ -5134,7 +5175,11 @@ case OP_OffsetLimit: { /* in1, out2, in3 */
>>>>> assert(pIn1->flags & (MEM_Int | MEM_UInt));
>>>>> assert(pIn3->flags & (MEM_Int | MEM_UInt));
>>>>> x = pIn1->u.i;
>>>>> - if (x<=0 || sqlAddInt64(&x, pIn3->u.i > 0 ? pIn3->u.i : 0)) {
>>>>> + int64_t rhs = pIn3->flags & MEM_Int ? 0 : pIn3->u.u;
>>>>> + bool unused;
>>>>> + if ((x == 0 || pIn1->flags & MEM_Int) ||
>>>>> + sql_add_int(x, pIn1->flags & MEM_Int, rhs, false,
>>>>
>>>> 14. If you get to this line, then (pIn1->flags & MEM_Int) is already
>>>> 0 and can be inlined.
>>>
>>> Wait, why? If x == 0 then pIn1->flags == MEM_UInt -
>>> we consider 0 as an unsigned value.
>>
>> Because you can only get to sql_add_int(), if x != 0 and
>> pIn1->flags & MEM_Int == 0. It is the C standard. In an
>> expression (a || b) 'b' is executed iff 'a' is false.
>
> My bad, never mind.
>
>> Looks like that place is not tested at all. The tests pass
>> regardless of how I call sql_add_int: with pIn1->flags & MEM_Int -> false
>> or true.
>
> In fact, the reason is the same as for OP_DecrJumpZero and
> OP_IfNotZero: P1 is a limit counter and P3 is an offset counter.
> Hence, both are >= 0, it is checked before these opcodes are
> executed. Added fix to the previous patch:
>
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index f864ef950..81005d14a 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4929,18 +4929,16 @@ case OP_IfPos: { /* jump, in1 */
> * Otherwise, r[P2] is set to the sum of r[P1] and r[P3].
> */
> case OP_OffsetLimit: { /* in1, out2, in3 */
> - i64 x;
> pIn1 = &aMem[pOp->p1];
> pIn3 = &aMem[pOp->p3];
> pOut = out2Prerelease(p, pOp);
> - assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
> - assert((pIn3->flags & (MEM_Int | MEM_UInt)) != 0);
> - x = pIn1->u.i;
> - int64_t rhs = pIn3->flags & MEM_Int ? 0 : pIn3->u.u;
> + assert((pIn1->flags & MEM_UInt) != 0);
> + assert((pIn3->flags & MEM_UInt) != 0);
> + uint64_t x = pIn1->u.u;
> + uint64_t rhs = pIn3->u.u;
> bool unused;
> - if ((x == 0 || pIn1->flags & MEM_Int) ||
> - sql_add_int(x, pIn1->flags & MEM_Int, rhs, false,
> - (int64_t *) &x, &unused) != 0) {
> + if (x == 0 || sql_add_int(x, false, rhs, false, (int64_t *) &x,
> + &unused) != 0) {
>
>> Please, inline the value (false), and add a test, which would fail, if
>> I put here true.
>>
I did this:
@@ -4943,7 +4943,7 @@ case OP_OffsetLimit: { /* in1, out2, in3 */
uint64_t x = pIn1->u.u;
uint64_t rhs = pIn3->u.u;
bool unused;
- if (x == 0 || sql_add_int(x, false, rhs, false, (int64_t *) &x,
+ if (x == 0 || sql_add_int(x, true, rhs, false, (int64_t *) &x,
&unused) != 0) {
/* If the LIMIT is less than or equal to zero, loop forever. This
* is documented. But also, if the LIMIT+OFFSET exceeds 2^63 then
And the tests passed. Looks like a test was not added.
next prev parent reply other threads:[~2019-07-10 22:48 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-07 15:37 [tarantool-patches] [PATCH 0/6] Introduce UNSIGNED type in SQL Nikita Pettik
2019-06-07 15:37 ` [tarantool-patches] [PATCH 1/6] sql: refactor sql_atoi64() Nikita Pettik
2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy
2019-07-01 14:20 ` n.pettik
2019-07-01 21:53 ` Vladislav Shpilevoy
2019-07-05 16:32 ` n.pettik
2019-06-07 15:37 ` [tarantool-patches] [PATCH 2/6] sql: separate VDBE memory holding positive and negative ints Nikita Pettik
2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy
2019-07-01 14:21 ` n.pettik
2019-07-01 21:53 ` Vladislav Shpilevoy
2019-07-05 16:33 ` n.pettik
2019-06-07 15:37 ` [tarantool-patches] [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints Nikita Pettik
2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy
2019-07-01 14:21 ` n.pettik
2019-07-01 21:53 ` Vladislav Shpilevoy
2019-07-05 16:36 ` n.pettik
2019-07-10 22:49 ` Vladislav Shpilevoy [this message]
2019-07-17 12:24 ` n.pettik
2019-06-07 15:37 ` [tarantool-patches] [PATCH 4/6] sql: make built-in functions operate on unsigned values Nikita Pettik
2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy
2019-07-01 14:21 ` n.pettik
2019-07-01 21:53 ` Vladislav Shpilevoy
2019-07-05 16:36 ` n.pettik
2019-07-10 22:49 ` Vladislav Shpilevoy
2019-07-17 0:53 ` n.pettik
2019-06-07 15:37 ` [tarantool-patches] [PATCH 5/6] sql: introduce extended range for INTEGER type Nikita Pettik
2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy
2019-07-01 14:21 ` n.pettik
2019-07-01 21:53 ` Vladislav Shpilevoy
2019-07-24 15:59 ` Konstantin Osipov
2019-07-24 16:54 ` n.pettik
2019-07-24 17:09 ` Konstantin Osipov
2019-06-07 15:37 ` [tarantool-patches] [PATCH 6/6] sql: allow to specify UNSIGNED column type Nikita Pettik
2019-07-01 21:53 ` [tarantool-patches] " Vladislav Shpilevoy
2019-07-05 16:36 ` n.pettik
2019-07-10 22:49 ` Vladislav Shpilevoy
2019-07-11 21:25 ` Vladislav Shpilevoy
2019-07-17 0:53 ` n.pettik
2019-07-18 20:18 ` Vladislav Shpilevoy
2019-07-18 20:56 ` n.pettik
2019-07-18 21:08 ` Vladislav Shpilevoy
2019-07-18 21:13 ` Vladislav Shpilevoy
2019-07-22 10:20 ` n.pettik
2019-07-22 19:17 ` Vladislav Shpilevoy
2019-07-22 10:20 ` n.pettik
2019-07-17 0:54 ` n.pettik
2019-07-18 20:18 ` Vladislav Shpilevoy
2019-08-06 19:36 ` n.pettik
2019-07-24 13:01 ` [tarantool-patches] Re: [PATCH 0/6] Introduce UNSIGNED type in SQL Kirill Yukhin
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=ba4b1598-867f-40f2-63c6-f16235043c55@tarantool.org \
--to=v.shpilevoy@tarantool.org \
--cc=korablev@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='[tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints' \
/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