From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 62BE9445320 for ; Tue, 7 Jul 2020 02:11:06 +0300 (MSK) Date: Mon, 6 Jul 2020 23:11:05 +0000 From: Nikita Pettik Message-ID: <20200706231104.GE22427@tarantool.org> References: <64653f49b79f2888243a76c30425d762acd301db.1593096639.git.imeevma@gmail.com> <20200629235129.GC27240@tarantool.org> <20200705144720.GD135859@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200705144720.GD135859@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3 6/8] sql: remove implicit cast for comparison List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org On 05 Jul 17:47, Mergen Imeev wrote: > My answers and new patch below. > > On Mon, Jun 29, 2020 at 11:51:29PM +0000, Nikita Pettik wrote: > > On 25 Jun 18:17, imeevma@tarantool.org wrote: > > > Thank you for review! My answers and new patch below. > > > > > > On 22.06.2020 15:25, Nikita Pettik wrote: > > > > On 17 Jun 15:36, imeevma@tarantool.org wrote: > > > >> @@ -2399,14 +2387,6 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ > > > >> default: res2 = res>=0; break; > > > >> } > > > >> > > > >> - /* Undo any changes made by mem_apply_type() to the input registers. */ > > > >> - assert((pIn1->flags & MEM_Dyn) == (flags1 & MEM_Dyn)); > > > >> - pIn1->flags = flags1; > > > >> - pIn1->field_type = ft_p1; > > > >> - assert((pIn3->flags & MEM_Dyn) == (flags3 & MEM_Dyn)); > > > >> - pIn3->flags = flags3; > > > >> - pIn3->field_type = ft_p3; > > > >> - > > > > > > > > Replace these assertions with relevant ones. > > > > > > > Not sure if this is necessary, since now we have much less flag > > > changes after removal of mem_apply_type(). > > > > Не понял как это связано...Твои изменения не затрагивают > > инвариант MEM_Dyn. > > > True, however original asserts checked that > mem_apply_type() does not changed it. Now we can say that > it definitely won't be changed. I can return flags* > variables for the sake of these asserts. Should I do this? > > Also, after #4906 these asserts become outdated since flags > of the mems won't change at all. Well, it will be so if we > close #4906. У этого тикета даже майлстоуна нет, если что. > > > @@ -4744,7 +4753,25 @@ case OP_IdxGE: { /* jump */ > > > assert(pOp->opcode==OP_IdxGE || pOp->opcode==OP_IdxLT); > > > r.default_rc = 0; > > > } > > > + > > > + /* > > > + * Make sure that the types of all the fields in the tuple > > > + * that will be used in the iterator comparable with the > > > + * fields of the space. > > > + */ > > > r.aMem = &aMem[pOp->p3]; > > > + for (int i = 0; i < r.nField; ++i) { > > > + enum field_type type = r.key_def->parts[i].type; > > > + struct Mem *mem = &r.aMem[i]; > > > > Как-то мне эти два места (выше в SeekGE) вообще не нравятся. > > Какие-то они запутанные. Давай подумаем как можно зарефакторить их. > > > Moved to a new function. However, previously cast > here didn't always worked since in some cases field_type > and MEM_type of the mem not always were compatible. I think > this was fixed in previous commits. All tests passes. > > However, there is another possiblity: it may be that I > already removed tests that fail on convertion but worked > on check. I do not think that it is the case though. Свят-свят. Давай подробнее. То что все тесты (которые перекромсали десять раз) ходят - не особо важно. Катить в релиз что-то над чем есть сомнения - не лучшая идея. > This also can be fully fixed in #4906 issue. Как?! Тот тикет про рефакторинг! > > > + if (mem_check_type(mem, type) == 0) > > > + continue; > > > + if (sql_type_is_numeric(type) && > > > + (mem->flags & (MEM_Real | MEM_Int | MEM_UInt)) != 0) > > > + continue; > > > + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > > > + field_type_strs[type], mem_type_to_str(mem)); > > > + goto abort_due_to_error; > > > + } > > > #ifdef SQL_DEBUG > > > { int i; for(i=0; i > > #endif > > > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > > > index 6d8768865..1d7c76670 100644 > > > --- a/src/box/sql/wherecode.c > > > +++ b/src/box/sql/wherecode.c > > > @@ -335,72 +335,6 @@ disableTerm(WhereLevel * pLevel, WhereTerm * pTerm) > > > } > > > } > > > > > > -/** > > > - * Code an OP_ApplyType opcode to apply the column type string > > > - * @types to the n registers starting at @base. > > > - * > > > - * As an optimization, SCALAR entries (which are no-ops) at the > > > - * beginning and end of @types are ignored. If all entries in > > > - * @types are SCALAR, then no code gets generated. > > > - * > > > - * This routine makes its own copy of @types so that the caller is > > > - * free to modify @types after this routine returns. > > > - */ > > > -static void > > > -emit_apply_type(Parse *pParse, int base, int n, enum field_type *types) > > > > Ты в следующем коммите как раз делаешь клин-ап. Давай эту функцию > > там и удалим (как и остальные). > > > It is a bit troublesome since the functions I remove here > are 'static'. I think, that instead of removing 'static' > keyword here and removing the function in the next commit > it is better to just remove these runctions here. > > I can change it if you think that removing in two commits > is better. > > commit f52e6669146e7a06b78d167abf369d67d9327789 > Author: Mergen Imeev > Date: Thu Jun 25 12:39:19 2020 +0300 > > sql: remove implicit cast for comparison > > This patch removes implicit cast for comparison. > > Closes #4230 > > @TarantoolBot document > Title: remove implicit cast for comparison > > After this patch-set, there will be no implicit casts for > comparison. This means that the values ​​of the field types STRING, > BOOLEAN and VARBINARY can be compared with the values ​​of the same Непечатные символы в тексте, проверь. > field type. Any numerical value can be compared with any other > numerical value. > > Example: > > For comparison: > ``` > tarantool> box.execute([[SELECT '1' > 0;]]) > --- > - null > - "Type mismatch: can not convert '1' (type: text) to numeric" > ... > > tarantool> box.execute([[SELECT true > X'33';]]) > --- > - null > - 'Type mismatch: can not convert ''TRUE'' (type: boolean) to varbinary' > ... > > tarantool> box.execute([[SELECT 1.23 > 123;]]) > --- > - metadata: > - name: 1.23 > 123 > type: boolean > rows: > - [false] > ... > ``` > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index ce9edaad9..0d3da49d3 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -561,6 +561,50 @@ mem_convert_to_numeric(struct Mem *mem, enum field_type type, bool is_precise) > return mem_convert_to_integer(mem, is_precise); > } > > +/** > + * Check that MEM types of mems are compatible with the > + * corresponding types from key_def. Так compatible или comparable? > + * If mem and type are comparable but incompatible according to > + * field_mp_plain_type_is_compatible() and the comparison > + * operation is EQ, we can say that the result of the comparison > + * will be FALSE. This allows us to compare DOUBLE and INTEGER Почему false? Не очевидно. > + * values. > + * > + * @param mems The first mem of the array. > + * @param def key_def that contains types to check. > + * @param size Size of the array. > + * @param is_op_eq TRUE is comparison operator is EQ. -> if. Как на результат совместимости типов влияет оператор сравнения? > + * @param[out] is_not_converted TRUE if operation is EQ and mem > + and type are comparable but not > + compatible. FALSE otherwise. Ты просто перечислил условия, при котором выставляешь этот флаг. Какой смысл в этом комментарии? > + * @retval TRUE if the MEM types and types from key_def are > + * comparable, FALSE otherwise. Ты про диаг забыл... > + */ > +static bool > +mem_are_types_comparable(struct Mem *mems, struct key_def *def, uint32_t size, > + bool is_op_eq, bool *is_not_converted) { Извини, но в этой функции плохо просто все. Давай по порядку: - название: почему mem_are? Откуда types, если там кей деф. Сайз чего? Что значит is_not_converted, если ты просто делаешь проверку на совместимость? - предполагается, что функция что-то проверяет и возвращает как следствие результат проверки (_are_). Но почему-то она еще и диаг выставляет. Не надо так. > + assert(!is_op_eq || !*is_not_converted); - зачем тебе два флага, которые друг на друга как-то нетривиально завязаны, причем в половине вызовов (1 из 2) они вообще не использу- ются? > + for (uint32_t i = 0; i < size; ++i) { - где ассерт i < def->part_count ? > + enum field_type type = def->parts[i].type; > + struct Mem *mem = &mems[i]; > + if (mem_is_type_compatible(mem, type)) > + continue; > + if (mem_convert_to_numeric(mem, type, true) == 0) > + continue; > + if (sql_type_is_numeric(type) && > + (mem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0) { > + if (is_op_eq) > + *is_not_converted = true; > + continue; > + } - тут вообще не понятно что происходит - тройной if с выставлением выходного параметра. Я честно говорю, что я не понимаю что тут происходит. sql_type_is_numeric() уже вызывается в mem_convert_to_numeric()... Давай на днях вместе сядем и подумаем как это все исправить. > + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > + field_type_strs[type], mem_type_to_str(mem)); > + return false; > + } > + return true; > +} > + > @@ -3623,7 +3645,20 @@ skip_truncate: > assert(oc!=OP_SeekGE || r.default_rc==+1); > assert(oc!=OP_SeekLT || r.default_rc==+1); > > + /* > + * Make sure that the types of all the fields in the tuple > + * that will be used in the iterator match the field types > + * of the space. > + */ > r.aMem = &aMem[pOp->p3]; > + bool is_not_found = false; Не очень логично, что функция проверяющая на совместимость типов, еще принимает флаг is_not_found..?? Что она ищет? > + if (!mem_are_types_comparable(r.aMem, r.key_def, r.nField, eqOnly == 1, > + &is_not_found)) > + goto abort_due_to_error; > + if (is_not_found) { > + res = 1; > + goto seek_not_found; > + } > #ifdef SQL_DEBUG > { int i; for(i=0; i #endif > @@ -4750,7 +4785,16 @@ case OP_IdxGE: { /* jump */ > assert(pOp->opcode==OP_IdxGE || pOp->opcode==OP_IdxLT); > r.default_rc = 0; > } > + > + /* > + * Make sure that the types of mems are comparable with > + * the field types of the space. > + */ > r.aMem = &aMem[pOp->p3]; > + bool unused = false; > + if (!mem_are_types_comparable(r.aMem, r.key_def, r.nField, false, > + &unused)) > + goto abort_due_to_error; > #ifdef SQL_DEBUG > { int i; for(i=0; i #endif