From: Mergen Imeev <imeevma@tarantool.org> To: "n.pettik" <korablev@tarantool.org> Cc: tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL. Date: Mon, 8 Jul 2019 13:13:37 +0300 [thread overview] Message-ID: <20190708101336.GA10686@tarantool.org> (raw) In-Reply-To: <F5517B43-5586-418B-A564-75A3006CA36B@tarantool.org> Hi! Thank you for review and fixes. I squashed you fixes and made a few changes. New patch and diff for these changes below. On Tue, Jul 02, 2019 at 02:29:14AM +0300, n.pettik wrote: > > > > On 28 Jun 2019, at 18:34, imeevma@tarantool.org wrote: > > > > Before this patch, UPDATE throwed an error when executed on a > > tuple with types unknown to SQL. This patch fixes the problem in > > case the fields with unknown types are not updated. > > I’ve fixed commit message a bit: > > sql: add ARRAY, MAP and ANY types to mem_apply_type() > > mem_apply_type() implements implicit type conversion. As a rule, tuple > to be inserted to the space is exposed to this conversion which is > invoked during execution of OP_MakeRecord opcode (which in turn forms > tuple). This function was not adjusted to operate on ARRAY, MAP and ANY > field types since they are poorly supported in current SQL > implementation. Hence, when tuple to be inserted in space having > mentioned field types reaches this function, it results in error. Note > that we can't set ARRAY or MAP types in SQL, but such situation may > appear during UPDATE operation on space created via Lua interface. This > problem is solved by extending implicit type conversions with obvious > casts: array field can be casted to array, map to map and any to any. > Thanks, fixed. > > Closes #4189 > > --- > > https://github.com/tarantool/tarantool/issues/4189 > > https://github.com/tarantool/tarantool/tree/imeevma/gh-4189-field-type-conversion-error > > I suggest follow refactoring: > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 8ddc29d14..f8cf1afc7 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -292,7 +292,9 @@ mem_apply_numeric_type(struct Mem *record) > * Convert mem to a string representation. > * > * SCALAR: > - * Mem is unchanged, but flag is set to BLOB. > + * Mem is unchanged, but flag is set to BLOB in case of > + * scalar-like type. Otherwise, (MAP, ARRAY) conversion > + * is impossible. > * > * BOOLEAN: > * If memory holds BOOLEAN no actions take place. > @@ -300,13 +302,9 @@ mem_apply_numeric_type(struct Mem *record) > * ANY: > * Mem is unchanged, no actions take place. > * > - * MAP: > - * If memory holds value with SQL_SUBTYPE_MSGPACK subtype and > - * data has MP_MAP type, no actions take place. > - * > - * ARRAY: > - * If memory holds value with SQL_SUBTYPE_MSGPACK subtype and > - * data has MP_ARRAY type, no actions take place. > + * MAP/ARRAY: > + * These types can't be casted to scalar ones, or to each > + * other. So the only valid conversion is to type itself. > * > * @param record The value to apply type to. > * @param type The type to be applied. > @@ -351,8 +349,13 @@ mem_apply_type(struct Mem *record, enum field_type type) > } > record->flags &= ~(MEM_Real | MEM_Int); > return 0; > - case FIELD_TYPE_ANY: > case FIELD_TYPE_SCALAR: > + /* Can't cast MAP and ARRAY to scalar types. */ > + if (record->subtype == SQL_SUBTYPE_MSGPACK) { > + assert(mp_typeof(*record->z) == MP_MAP || > + mp_typeof(*record->z) == MP_ARRAY); > + return -1; > + } > return 0; > case FIELD_TYPE_MAP: > if (record->subtype == SQL_SUBTYPE_MSGPACK && > @@ -364,6 +367,8 @@ mem_apply_type(struct Mem *record, enum field_type type) > mp_typeof(*record->z) == MP_ARRAY) > return 0; > return -1; > + case FIELD_TYPE_ANY: > + return 0; > default: > return -1; > } > diff --git a/test/sql/types.result b/test/sql/types.result > index 5cc7f1659..32588d38c 100644 > --- a/test/sql/types.result > +++ b/test/sql/types.result > @@ -967,8 +967,8 @@ box.execute('SELECT ?', {true}) > - [true] > ... > -- > --- gh-4189: Update throws an error when executed on a tuple with > --- types unknown to SQL. > +-- gh-4189: make sure that update doesn't throw an error if format > +-- of table features map/array field types. > -- > format = {} > --- > diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua > index d33314a0b..69af421d2 100644 > --- a/test/sql/types.test.lua > +++ b/test/sql/types.test.lua > @@ -236,8 +236,8 @@ box.execute('SELECT \'9223372036854\' + 1;') > box.execute('SELECT ?', {true}) > > -- > --- gh-4189: Update throws an error when executed on a tuple with > --- types unknown to SQL. > +-- gh-4189: make sure that update doesn't throw an error if format > +-- of table features map/array field types. > -- > format = {} > format[1] = {type = 'integer', name = 'I'} > Thanks, squashed. Diff: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index f8cf1af..79232de 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -351,19 +351,22 @@ mem_apply_type(struct Mem *record, enum field_type type) return 0; case FIELD_TYPE_SCALAR: /* Can't cast MAP and ARRAY to scalar types. */ - if (record->subtype == SQL_SUBTYPE_MSGPACK) { + if ((record->flags & MEM_Blob) == MEM_Blob && + record->subtype == SQL_SUBTYPE_MSGPACK) { assert(mp_typeof(*record->z) == MP_MAP || mp_typeof(*record->z) == MP_ARRAY); return -1; } return 0; case FIELD_TYPE_MAP: - if (record->subtype == SQL_SUBTYPE_MSGPACK && + if ((record->flags & MEM_Blob) == MEM_Blob && + record->subtype == SQL_SUBTYPE_MSGPACK && mp_typeof(*record->z) == MP_MAP) return 0; return -1; case FIELD_TYPE_ARRAY: - if (record->subtype == SQL_SUBTYPE_MSGPACK && + if ((record->flags & MEM_Blob) == MEM_Blob && + record->subtype == SQL_SUBTYPE_MSGPACK && mp_typeof(*record->z) == MP_ARRAY) return 0; return -1; New patch: From 5c04d08a5d6c88ea25b60b931e83bfedde5ae27e Mon Sep 17 00:00:00 2001 Date: Fri, 28 Jun 2019 18:16:16 +0300 Subject: [PATCH] sql: add ARRAY, MAP and ANY types to mem_apply_type() Function mem_apply_type() implements implicit type conversion. As a rule, tuple to be inserted to the space is exposed to this conversion which is invoked during execution of OP_MakeRecord opcode (which in turn forms tuple). This function was not adjusted to operate on ARRAY, MAP and ANY field types since they are poorly supported in current SQL implementation. Hence, when tuple to be inserted in space having mentioned field types reaches this function, it results in error. Note that we can't set ARRAY or MAP types in SQL, but such situation may appear during UPDATE operation on space created via Lua interface. This problem is solved by extending implicit type conversions with obvious casts: array field can be casted to array, map to map and any to any. Closes #4189 diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index c8887f9..79232de 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -292,7 +292,19 @@ mem_apply_numeric_type(struct Mem *record) * Convert mem to a string representation. * * SCALAR: - * Mem is unchanged, but flag is set to BLOB. + * Mem is unchanged, but flag is set to BLOB in case of + * scalar-like type. Otherwise, (MAP, ARRAY) conversion + * is impossible. + * + * BOOLEAN: + * If memory holds BOOLEAN no actions take place. + * + * ANY: + * Mem is unchanged, no actions take place. + * + * MAP/ARRAY: + * These types can't be casted to scalar ones, or to each + * other. So the only valid conversion is to type itself. * * @param record The value to apply type to. * @param type The type to be applied. @@ -338,6 +350,27 @@ mem_apply_type(struct Mem *record, enum field_type type) record->flags &= ~(MEM_Real | MEM_Int); return 0; case FIELD_TYPE_SCALAR: + /* Can't cast MAP and ARRAY to scalar types. */ + if ((record->flags & MEM_Blob) == MEM_Blob && + record->subtype == SQL_SUBTYPE_MSGPACK) { + assert(mp_typeof(*record->z) == MP_MAP || + mp_typeof(*record->z) == MP_ARRAY); + return -1; + } + return 0; + case FIELD_TYPE_MAP: + if ((record->flags & MEM_Blob) == MEM_Blob && + record->subtype == SQL_SUBTYPE_MSGPACK && + mp_typeof(*record->z) == MP_MAP) + return 0; + return -1; + case FIELD_TYPE_ARRAY: + if ((record->flags & MEM_Blob) == MEM_Blob && + record->subtype == SQL_SUBTYPE_MSGPACK && + mp_typeof(*record->z) == MP_ARRAY) + return 0; + return -1; + case FIELD_TYPE_ANY: return 0; default: return -1; diff --git a/test/sql/types.result b/test/sql/types.result index cdfb1e7..32588d3 100644 --- a/test/sql/types.result +++ b/test/sql/types.result @@ -966,3 +966,46 @@ box.execute('SELECT ?', {true}) rows: - [true] ... +-- +-- gh-4189: make sure that update doesn't throw an error if format +-- of table features map/array field types. +-- +format = {} +--- +... +format[1] = {type = 'integer', name = 'I'} +--- +... +format[2] = {type = 'boolean', name = 'B'} +--- +... +format[3] = {type = 'array', name = 'F1'} +--- +... +format[4] = {type = 'map', name = 'F2'} +--- +... +format[5] = {type = 'any', name = 'F3'} +--- +... +s = box.schema.space.create('T', {format = format}) +--- +... +ii = s:create_index('ii') +--- +... +s:insert({1, true, {1, 2}, {a = 3}, 'asd'}) +--- +- [1, true, [1, 2], {'a': 3}, 'asd'] +... +box.execute('UPDATE t SET b = false WHERE i = 1;') +--- +- row_count: 1 +... +s:select() +--- +- - [1, false, [1, 2], {'a': 3}, 'asd'] +... +s:drop() +--- +... diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua index ae1a0ab..69af421 100644 --- a/test/sql/types.test.lua +++ b/test/sql/types.test.lua @@ -234,3 +234,20 @@ box.execute('SELECT \'9223372036854\' + 1;') -- Fix BOOLEAN bindings. box.execute('SELECT ?', {true}) + +-- +-- gh-4189: make sure that update doesn't throw an error if format +-- of table features map/array field types. +-- +format = {} +format[1] = {type = 'integer', name = 'I'} +format[2] = {type = 'boolean', name = 'B'} +format[3] = {type = 'array', name = 'F1'} +format[4] = {type = 'map', name = 'F2'} +format[5] = {type = 'any', name = 'F3'} +s = box.schema.space.create('T', {format = format}) +ii = s:create_index('ii') +s:insert({1, true, {1, 2}, {a = 3}, 'asd'}) +box.execute('UPDATE t SET b = false WHERE i = 1;') +s:select() +s:drop()
next prev parent reply other threads:[~2019-07-08 10:13 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-28 15:34 [tarantool-patches] " imeevma 2019-07-01 23:29 ` [tarantool-patches] " n.pettik 2019-07-08 10:13 ` Mergen Imeev [this message] 2019-07-08 13:32 ` n.pettik 2019-07-09 9:14 ` Imeev Mergen 2019-07-15 13:32 ` n.pettik 2019-07-16 14:19 ` Mergen Imeev 2019-07-18 15:59 ` n.pettik
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=20190708101336.GA10686@tarantool.org \ --to=imeevma@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL.' \ /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