From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type Date: Mon, 22 Jul 2019 13:20:14 +0300 [thread overview] Message-ID: <F7B17DF8-B2F6-43BA-9AA3-AEFACF6AB134@tarantool.org> (raw) In-Reply-To: <989f9710-043f-447a-0cc4-76eb317bc1e9@tarantool.org> > On 19 Jul 2019, at 00:08, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > On 18/07/2019 22:56, n.pettik wrote: >> >>> On 18 Jul 2019, at 23:18, Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>> wrote: >>>>> ------------------------- >>>>> 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. So, you want to see smth like this: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 73a1f321b..835544d44 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -306,8 +306,6 @@ mem_apply_type(struct Mem *record, enum field_type type) switch (type) { 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) { @@ -317,7 +315,14 @@ mem_apply_type(struct Mem *record, enum field_type type) record->u.r <= -1); return 0; } - return sqlVdbeMemIntegerify(record, false); + if (sqlVdbeMemIntegerify(record, false) != 0) + return -1; + if ((record->flags & MEM_Int) == MEM_Int) { + if (type == FIELD_TYPE_UNSIGNED) + return -1; + return 0; + } + return 0; The difference can be seen in queries like this: box.execute("CREATE TABLE t1 (id UNSIGNED PRIMARY KEY);") box.execute("INSERT INTO t1 VALUES (-3);") --- - error: 'Type mismatch: can not convert -3 to unsigned' … Without this change we got: 'Tuple field 1 type does not match one required by operation: expected unsigned’ I consider this check to be a bit inappropriate since It is redundant. Comparison between unsigned and signed is well defined; insertion to the unsigned field is prohibited and checked in more low level core mechanisms. I would say I don’t mind applying this change (and already applied to speed up review process), but on the other side I don’t understand why do we need to add extra checks on top (SQL) layer.
next prev parent reply other threads:[~2019-07-22 10:20 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 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 [this message] 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=F7B17DF8-B2F6-43BA-9AA3-AEFACF6AB134@tarantool.org \ --to=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type' \ /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