From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 4E2C042EF5C for ; Mon, 22 Jun 2020 15:25:51 +0300 (MSK) Date: Mon, 22 Jun 2020 12:25:50 +0000 From: Nikita Pettik Message-ID: <20200622122550.GE30686@tarantool.org> References: <61c17e9c8cd56993acaf2eb4395769e67c81ec44.1592397263.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <61c17e9c8cd56993acaf2eb4395769e67c81ec44.1592397263.git.imeevma@gmail.com> Subject: Re: [Tarantool-patches] [PATCH v2 5/7] sql: remove implicit cast from string 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 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. > @@ -3496,8 +3476,6 @@ case OP_SeekGT: { /* jump, in3 */ > pIn3 = &aMem[int_field]; > if ((pIn3->flags & MEM_Null) != 0) > goto skip_truncate; > - if ((pIn3->flags & MEM_Str) != 0) > - mem_apply_numeric_type(pIn3); > int64_t i; > if ((pIn3->flags & MEM_Int) == MEM_Int) { > i = pIn3->u.i; Could you substitute routine below with mem_check_types() (or whatever this function is called)? > @@ -3590,6 +3568,26 @@ skip_truncate: > assert(oc!=OP_SeekLT || r.default_rc==+1); > > r.aMem = &aMem[pOp->p3]; Code below definitely lacks some comments. > + 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->flags & MEM_Str) != 0 && sql_type_is_numeric(type)) { What if key_def contains scalar/any type? > + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > + field_type_strs[type], mem_type_to_str(mem)); > + goto abort_due_to_error; > + } > + if (mem_check_types(mem, type) == 0) > + continue; > + if ((mem->flags & MEM_Real) != 0 && > + (type == FIELD_TYPE_INTEGER || > + type == FIELD_TYPE_UNSIGNED)) { > + 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 > @@ -4717,6 +4715,27 @@ case OP_IdxGE: { /* jump */ > r.default_rc = 0; > } > r.aMem = &aMem[pOp->p3]; > + for (int i = 0; i < r.nField; ++i) { > + struct Mem *mem = &r.aMem[i]; > + enum mp_type mp_type = sql_value_type(mem); > + enum field_type field_type = r.key_def->parts[i].type; > + if (field_type == FIELD_TYPE_SCALAR || > + mem->field_type == FIELD_TYPE_SCALAR) > + continue; > + bool is_nullable = r.key_def->parts[i].nullable_action == > + ON_CONFLICT_ACTION_NONE; > + if (field_mp_plain_type_is_compatible(field_type, mp_type, > + is_nullable)) > + continue; Why this procedure is different from one in Seek* opcode handling routine? Could you put it in one function and reuse? Also lacks comments. > + if (!sql_type_is_numeric(field_type) || > + !(mp_type == MP_INT || mp_type == MP_UINT || > + mp_type == MP_DOUBLE || mp_type == MP_FLOAT)) { > + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > + mem_type_to_str(mem), > + field_type_strs[field_type]); > + goto abort_due_to_error; > + } > + } > 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) > } > } > > -/** > - types[i] = FIELD_TYPE_SCALAR; > - } > - } > -} > - > /* > * Generate code for a single equality term of the WHERE clause. An equality > * term can be either X=expr or X IN (...). pTerm is the term to be > testcase(pRangeStart->wtFlags & TERM_VIRTUAL); > if (sqlExprIsVector(pRight) == 0) { > @@ -1049,94 +950,6 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W > } > struct index_def *idx_pk = space->index[0]->def; > uint32_t pk_part_count = idx_pk->key_def->part_count; > - /* > - * Tarantool's iterator over integer fields doesn't > - * tolerate floating point values. Hence, if term > - * is equality comparison and value of operand is > - * not integer, we can skip it since it always > - * results in false: INT a == 0.5 -> false; > - * It is done using OP_MustBeInt facilities. > - * In case term is greater comparison (a > ?), we > - * should notify OP_SeekGT to process truncation of > - * floating point value: a > 0.5 -> a >= 1; > - * It is done by setting P5 flag for OP_Seek*. > - * It is worth mentioning that we do not need > - * this step when it comes for less (<) comparison > - * of nullable field. Key is NULL in this case: > - * values are ordered as NULL, ... NULL, min_value, > - * so to fetch min value we pass NULL to GT iterator. > - * The only exception is less comparison in > - * conjunction with ORDER BY DESC clause: > - * in such situation we use LE iterator and > - * truncated value to compare. But then > - * pRangeStart == NULL. > - * This procedure is correct for compound index: > - * only one comparison of less/greater type can be > - * used at the same time. For instance, > - * a < 1.5 AND b > 0.5 is handled by SeekGT using > - * column a and fetching column b from tuple and > - * OP_Le comparison. > - * > - * Note that OP_ApplyType, which is emitted before > - * OP_Seek** doesn't truncate floating point to > - * integer. That's why we need this routine. > - * Also, note that terms are separated by OR > - * predicates, so we consider term as sequence > - * of AND'ed predicates. > - */ > - size_t addrs_sz; > - int *seek_addrs = region_alloc_array(&pParse->region, > - typeof(seek_addrs[0]), nEq, > - &addrs_sz); > - if (seek_addrs == NULL) { > - diag_set(OutOfMemory, addrs_sz, "region_alloc_array", > - "seek_addrs"); > - pParse->is_aborted = true; > - return 0; Solid code removal. I won't review it, I hope you verified that all tests are working as desired. > - memset(seek_addrs, 0, addrs_sz); > - for (int i = 0; i < nEq; i++) { > - enum field_type type = idx_def->key_def->parts[i].type; > - if (type == FIELD_TYPE_INTEGER || > - type == FIELD_TYPE_UNSIGNED) { > - /* > - * OP_MustBeInt consider NULLs as > - * non-integer values, so firstly > - * check whether value is NULL or not. > - */ > - seek_addrs[i] = sqlVdbeAddOp1(v, OP_IsNull, > - regBase); > - sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i, > - addrNxt); > - start_types[i] = FIELD_TYPE_SCALAR; > - /* > - * We need to notify column cache > - * that type of value may change > - * so we should fetch value from > - * tuple again rather then copy > - * from register. > - */ > - sql_expr_type_cache_change(pParse, regBase + i, > - 1); > - } > - } > - /* Inequality constraint comes always at the end of list. */ > - part_count = idx_def->key_def->part_count; > - if (pRangeStart != NULL) { > - /* > - * nEq == 0 means that filter condition > - * contains only inequality. > - */ > - uint32_t ineq_idx = nEq == 0 ? 0 : nEq - 1; > - assert(ineq_idx < part_count); > - enum field_type ineq_type = > - idx_def->key_def->parts[ineq_idx].type; > - if (ineq_type == FIELD_TYPE_INTEGER || > - ineq_type == FIELD_TYPE_UNSIGNED) > - force_integer_reg = regBase + nEq; > - } > - emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull, > - start_types); > if (pLoop->nSkip > 0 && nConstraint == pLoop->nSkip) { > /* The skip-scan logic inside the call to codeAllEqualityConstraints() > * above has already left the cursor sitting on the correct row, > --- /dev/null > +++ b/test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua Same note: I'd better use .sql test format and/or amalgamate it with already existing test file. > @@ -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?