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 2938C231AC for ; Thu, 18 Jul 2019 17:12:21 -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 gcKBBW-lRJUC for ; Thu, 18 Jul 2019 17:12:21 -0400 (EDT) Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 D446323103 for ; Thu, 18 Jul 2019 17:12:20 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type From: Vladislav Shpilevoy 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> <127420CE-540E-439C-B2BD-20007EE98328@tarantool.org> <989f9710-043f-447a-0cc4-76eb317bc1e9@tarantool.org> Message-ID: Date: Thu, 18 Jul 2019 23:13:59 +0200 MIME-Version: 1.0 In-Reply-To: <989f9710-043f-447a-0cc4-76eb317bc1e9@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik" , tarantool-patches@freelists.org Why are you keep trying not to fix this obvious bug? If that path is unreachable, then either delete it, or add an assertion. Or fix this place. On 18/07/2019 23:08, Vladislav Shpilevoy wrote: > > > On 18/07/2019 22:56, n.pettik wrote: >> >> >>> On 18 Jul 2019, at 23:18, Vladislav Shpilevoy > wrote: >>> >>> Hi! >>> >>> Thanks for the fixes! >>> >>>>> ------------------------- >>>>> vdbe.c:307 >>>>> >>>>>> case FIELD_TYPE_INTEGER: >>>>>> case FIELD_TYPE_UNSIGNED: >>>>>> if ((record->flags & MEM_Int) == MEM_Int) >>>>>> return 0; >>>>>> if ((record->flags & MEM_UInt) == MEM_UInt) >>>>>> return 0; >>>>>> if ((record->flags & MEM_Real) == MEM_Real) { >>>>>> int64_t i = (int64_t) record->u.r; >>>>>> if (i == record->u.r) >>>>>> mem_set_int(record, record->u.r, >>>>>>     record->u.r <= -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’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’s 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 :) >> > > But it is not implicit. It just does not work, when a value is negative, > and type is unsigned. >