From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 D0AFE41D952 for ; Tue, 30 Jun 2020 02:51:30 +0300 (MSK) Date: Mon, 29 Jun 2020 23:51:29 +0000 From: Nikita Pettik Message-ID: <20200629235129.GC27240@tarantool.org> References: <64653f49b79f2888243a76c30425d762acd301db.1593096639.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <64653f49b79f2888243a76c30425d762acd301db.1593096639.git.imeevma@gmail.com> 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: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org 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. > >> @@ -0,0 +1,281 @@ > >> +#!/usr/bin/env tarantool > >> +test = require("sqltester") > >> +test:plan(32) > >> + > >> +-- > >> + > >> +test:execsql([[ > >> + CREATE TABLE t1(x TEXT primary key); > >> + INSERT INTO t1 VALUES('1'); > >> + CREATE TABLE t2(x INTEGER primary key); > >> + INSERT INTO t2 VALUES(1); > >> + CREATE TABLE t3(x DOUBLE primary key); > >> + INSERT INTO t3 VALUES(1.0); > > > > What about table with scalar/any fields? > > > I think this is too big a problem to solve it here. Гораздо лучше потом клепать фоллоу-апы, точечно затыкая креши.. Я бы добавил хотя бы самых базовых примеров. > i = pIn3->u.i; > @@ -3617,7 +3595,38 @@ 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]; > + for (int i = 0; i < r.nField; ++i) { > + enum field_type type = r.key_def->parts[i].type; > + struct Mem *mem = &r.aMem[i]; > + if (mem_check_type(mem, type) == 0 || > + mem_convert_numeric(mem, type, true) == 0) > + continue; Я бы все-таки разбил бы эту проверку на два условия: if (...) continue if (...) continue А еще лучше вынести этот чанк в хелпер. > + /* > + * If the number was not converted without loss, > + * we will not find tuples using the EQ iterator. > + */ Пожайлуста, не используй двойное отрицание - комментарий очень сложно воспринимать. Давай это переформулируем, можно пример добавить. Это довольно скользкое место. > + if ((mem->flags & MEM_Real) != 0 && > + (type == FIELD_TYPE_INTEGER || > + type == FIELD_TYPE_UNSIGNED)) { > + assert(eqOnly == 1); > + res = 1; > + goto seek_not_found; > + } > + if ((mem->flags & (MEM_Int | MEM_UInt)) != 0 && > + type == FIELD_TYPE_DOUBLE && eqOnly == 1) { > + res = 1; > + goto seek_not_found; > + } > + 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 > @@ -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) вообще не нравятся. Какие-то они запутанные. Давай подумаем как можно зарефакторить их. > + 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) Ты в следующем коммите как раз делаешь клин-ап. Давай эту функцию там и удалим (как и остальные). > nConstraint++; > } > - sqlDbFree(db, start_types); > - sqlDbFree(db, end_types); > > /* Top of the loop body */ > pLevel->p2 = sqlVdbeCurrentAddr(v); > diff --git a/test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua b/test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua > new file mode 100755 > index 000000000..b405a11b6 > --- /dev/null > +++ b/test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua Когда кто-то ищет где посмотреть тесты связанные с типами, первое что приходит в голову - грепнуть *types*. Этот тест по сути ничего и не тестирует (уверен, эти пути уже покрыты в других тестах), зато хорошо отображает работу системы типов. Я бы просто подумал о других разработчиках, и все же замержил его в какой-нибудь тест типа sql/types.test.lua.