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 185142458A for ; Thu, 18 Jul 2019 16:56:52 -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 OwxOhFQX3wQa for ; Thu, 18 Jul 2019 16:56:52 -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 A74E624588 for ; Thu, 18 Jul 2019 16:56:51 -0400 (EDT) From: "n.pettik" Message-Id: <127420CE-540E-439C-B2BD-20007EE98328@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_1EF03C2D-3307-4727-B1C7-D1D845D59D72" Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type Date: Thu, 18 Jul 2019 23:56:48 +0300 In-Reply-To: <8e4feefd-7bfb-18af-fd0f-b45384e5d2ef@tarantool.org> References: <734EC309-6DCF-42C2-8041-135A8B68E935@tarantool.org> <9a397d31-1cae-0dd0-cdd6-733388cb01af@tarantool.org> <552F96C1-DAC5-4F18-9F5A-BF50C6BBC205@tarantool.org> <8e4feefd-7bfb-18af-fd0f-b45384e5d2ef@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 --Apple-Mail=_1EF03C2D-3307-4727-B1C7-D1D845D59D72 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 18 Jul 2019, at 23:18, Vladislav Shpilevoy = wrote: >=20 > Hi! >=20 > Thanks for the fixes! >=20 >>> ------------------------- >>> vdbe.c:307 >>>=20 >>>> case FIELD_TYPE_INTEGER: >>>> case FIELD_TYPE_UNSIGNED: >>>> if ((record->flags & MEM_Int) =3D=3D MEM_Int) >>>> return 0; >>>> if ((record->flags & MEM_UInt) =3D=3D MEM_UInt) >>>> return 0; >>>> if ((record->flags & MEM_Real) =3D=3D MEM_Real) { >>>> int64_t i =3D (int64_t) record->u.r; >>>> if (i =3D=3D record->u.r) >>>> mem_set_int(record, record->u.r, >>>> record->u.r <=3D -1); >>>> return 0; >>>> } >>>=20 >>> It is a part of function mem_apply_type. When target type is >>> UNSIGNED, and a value is MEM_Int, you do nothing. Why? Looks like >>> it is possible to pass here a negative value, and CAST UNSIGNED >>> would do nothing. >>=20 >> Basically, this function implements sort of implicit cast >> which takes place before comparison/assignment. >> For comparisons it makes no sense - we can compare >> integer with unsigned value - the latter is always greater. >> For assignment it is also meaningless: if we attempt >> at inserting negative values to unsigned field appropriate >> error will be raised anyway. If you can come up with >> specific example, let=E2=80=99s discuss it. >>=20 >=20 > I can't provide a test. But the function is named mem_apply_type, > and it doesn't apply type, when it is unsigned, and a value is > negative. Doesn't it look wrong to you? >=20 > If some code wants to get an integer, it can apply FIELD_TYPE_INTEGER > instead of FIELD_TYPE_UNSIGNED. IMO, an attempt to apply unsigned > to int should raise an error here. Otherwise this function can't > be named 'apply_type' because it ignores negative -> unsigned case. Okay, let=E2=80=99s rename it. I can suggest these options: mem_cast_implicit() mem_cast_implicit_to_type() mem_implicit_cast_to_type() mem_convert_implicit() mem_convert_to_type() mem_type_coerce_implicit() mem_type_implicit_coercion() mem_type_coercion_implicit() mem_implicit_type_juggling() mem_implicit_juggle_to_type() mem_do_implicit_conversion() mem_do_implicit_coercion() Or any other combination :) --Apple-Mail=_1EF03C2D-3307-4727-B1C7-D1D845D59D72 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On 18 Jul 2019, at 23:18, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:

Hi!

Thanks for the fixes!

-------------------------
vdbe.c:307

case = FIELD_TYPE_INTEGER:
case FIELD_TYPE_UNSIGNED:
= = if ((record->flags & MEM_Int) =3D=3D MEM_Int)
= = = return 0;
if ((record->flags & = MEM_UInt) =3D=3D MEM_UInt)
return 0;
if = ((record->flags & MEM_Real) =3D=3D MEM_Real) {
int64_t i = =3D (int64_t) record->u.r;
if (i =3D=3D record->u.r)
= = = = mem_set_int(record, record->u.r,
    record->= u.r <=3D -1);
return 0;
}

It is a part of function = mem_apply_type. When target type is
UNSIGNED, and a value = is MEM_Int, you do nothing. Why? Looks like
it is possible = to pass here a negative value, and CAST UNSIGNED
would do = nothing.

Basically, this = function implements sort of implicit cast
which takes = place before comparison/assignment.
For comparisons it = makes no sense - we can compare
integer with unsigned = value - the latter is always greater.
For assignment it is = also meaningless: if we attempt
at inserting negative = values to unsigned field appropriate
error will be raised = anyway. If you can come up with
specific example, let=E2=80=99= s discuss it.


I can't provide a test. But the = function is named mem_apply_type,
and it doesn't apply type, when it is unsigned, and a value = is
negative. = Doesn't it look wrong to you?

If some code wants to get an integer, it can apply = FIELD_TYPE_INTEGER
instead of FIELD_TYPE_UNSIGNED. IMO, an attempt to apply = unsigned
to int should = raise an error here. Otherwise this function can't
be named 'apply_type' because it = ignores negative -> unsigned case.

Okay, let=E2=80=99s rename it. I can suggest these = options:

mem_cast_implicit()
mem_cast_implicit_to_t= ype()
mem_implicit_cast_to_type()
mem_convert_implicit= ()
mem_convert_to_type()
mem_type_coerce_implicit()
mem_type_implicit_coercion()
mem_type_coercion_implicit(= )
mem_implicit_type_juggling()
mem_implicit_juggle_to_= type()
mem_do_implicit_conversion()
mem_do_implicit_co= ercion()

Or any other combination = :)

= --Apple-Mail=_1EF03C2D-3307-4727-B1C7-D1D845D59D72--