From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E168E2A58D for ; Thu, 1 Nov 2018 22:36:56 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9YiizJFsRCCX for ; Thu, 1 Nov 2018 22:36:56 -0400 (EDT) Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 36B8B2A39B for ; Thu, 1 Nov 2018 22:36:56 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH 3/6] sql: pass true types of columns to Tarantool From: "n.pettik" In-Reply-To: <2d06ce1c-bd65-221c-6946-bce549796925@tarantool.org> Date: Fri, 2 Nov 2018 05:36:47 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <9a94105515bd4f9148f302b24230d0918cfccdf9.1537216078.git.korablev@tarantool.org> <723E1B3C-AF45-4129-9C39-B5B2B04E3B28@tarantool.org> <2d06ce1c-bd65-221c-6946-bce549796925@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > On 30 Oct 2018, at 00:32, Vladislav Shpilevoy = wrote: >=20 > Hi! Thanks for the fixes! See 2 comments below. >=20 >>>> + struct index_def *pk =3D pUseIndex; >>>> assert(pk !=3D NULL); >>>> - uint32_t fieldno =3D pk->def->key_def->parts[0].fieldno; >>>> + uint32_t fieldno =3D pk->key_def->parts[0].fieldno; >>>> enum affinity_type affinity =3D >>>> tab->def->fields[fieldno].affinity; >>>> - if (pk->def->key_def->part_count =3D=3D 1 && >>>> + if (pk->key_def->part_count =3D=3D 1 && >>>> affinity =3D=3D AFFINITY_INTEGER && (int)fieldno < = nVector) { >>>> int reg_pk =3D rLhs + (int)fieldno; >>>=20 >>> 3. Why do we still need this MustBeInt here? As I understand, here = we >>> are trying to use an index for binary search of an LHS, but under = the >>> hood it is just tarantool iterators starting from certain keys, it = is >>> not? If it is, then tarantool iterators does key validation for us. = Can >>> you please check if this opcode is redundant? >> It doesn=E2=80=99t seem to be useless. Consider example: >> CREATE TABLE t1 (id INT PRIMARY KEY); >> SELECT 1.23 IN t1; >> Now, OP_Affinity doesn=E2=80=99t cast REAL to INT - I described = reason >> in previous review. So, without OP_MustBeInt query above would >> result in >> Execution failed: Supplied key type of part 0 does not match index = part type: expected integer >> Since 1.23 is passed to iterator over integer field. But I guess = query above >> is fine - it should simply return 0, not an error since queries like >> SELECT * FROM t1 WHERE id =3D 1.123; >> SELECT * FROM t1 WHERE id > 1.123; >> INSERT INTO t1 VALUES (1.123); >> are allowed as well. >=20 > 1. Then I am confused. Look at OP_MustBeInt. It calls > applyAffinity(pIn1, AFFINITY_NUMERIC). NUMERIC is real, so > 1.23 cast to NUMERIC should be unchanged, it is not? I > tried this: >=20 > tarantool> box.sql.execute("select CAST(1.23 as NUMERIC)") > --- > - - [1.23] > ... >=20 > So 1.23 is already numeric. But applyAffinity function > for unknown reason does this: for target affinity =3D=3D > NUMERIC it sees that the original affinity is real and > casts the result to integer. So > applyAffinity(1.23, NUMERIC) =3D=3D 1, but > SELECT CAST 1.23 as NUMERIC =3D=3D 1.23. ?! Sorry for making you confused, but this conversion takes place in the next patch: there it is not applyAffinity(1.23, NUMERIC), but mem_apply_affinity(1.23, INTEGER). So it seems to work. >>>> diff --git a/test/sql-tap/boundary3.test.lua = b/test/sql-tap/boundary3.test.lua >>>> index 5b63e0539..c5d605b65 100755 >>>> --- a/test/sql-tap/boundary3.test.lua >>>> +++ b/test/sql-tap/boundary3.test.lua >>>> @@ -125,7 +125,7 @@ test:do_test( >>>> -- >>>> }) >>>> -test:do_execsql_test( >>>> +--[[test:do_execsql_test( >>>=20 >>> 4. Do not silently comment tests. We should either delete it, or = fix, or >>> open an issue to fix it later. >> These tests are working. In previous review you asked to enable tests = which >> are under =E2=80=98if false=E2=80=99, so I forgot about simply = commented tests. >=20 > 2. Sorry, in3.test.lua still has a commented test. orderby5.test.lua = too. Overall, I=E2=80=99ve scrolled over patch again and tried to eliminate = all stray diffs and accidentally commented tests. Diff is really huge, I hope I didn=E2=80= =99t miss smth else again. I=E2=80=99ve deleted commented test in orderby5.test.lua since it = doesn=E2=80=99t really make any sense. Yep, query plan has changed (a lot of queries have changed their plans after types were specified for tables). But it is = still the same if I create table with types on current 2.1. diff --git a/test/sql-tap/orderby5.test.lua = b/test/sql-tap/orderby5.test.lua index 8a423843a..048f024ab 100755 --- a/test/sql-tap/orderby5.test.lua +++ b/test/sql-tap/orderby5.test.lua @@ -179,20 +179,6 @@ test:do_execsql_test( -- EXPLAIN QUERY PLAN -- SELECT * FROM t1 WHERE a=3D0 ORDER BY c, b, a; -- } {/B-TREE/} ---test:do_execsql_test( --- 3.0, --- [[ --- CREATE TABLE t3(a INTEGER PRIMARY KEY, b INT, c INT, d INT, e = INT, f INT); - --CREATE INDEX t3bcde ON t3(b, c, d, e); - -- As pk is not necessary in Tarantool's secondary indexes 'a' = should be added manually --- CREATE INDEX t3bcde ON t3(b, c, d, e, a); --- EXPLAIN QUERY PLAN --- SELECT a FROM t3 WHERE b=3D2 AND c=3D3 ORDER BY d DESC, e = DESC, b, c, a DESC; --- ]], { - -- <3.0> --- "~/B-TREE/" - -- --- }) =20 For in3.test.lua I=E2=80=99ve uncommented the rest of tests. However, = I=E2=80=99ve found a bug connected with IN operator: if query in form of SELECT * FROM t1 WHERE X IN (SELECT Y FROM t2); and PK of Y is of type INT, then additional OP_MustBeINT opcode is generated. But instead of PK, it is applied to column Y, which might not be forced to INT. Hence, I slightly refactored code for IN operator and removed generation of OP_MustBeINT at all. Instead, I allowed using index search only if types match exactly (what I think is the only right way). Also, I=E2=80=99ve added explanation comment for each = changed test in in3.test.lua diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 270a6664c..3dd7fb01a 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -2404,23 +2404,12 @@ sqlite3FindInIndex(Parse * pParse, /* = Parsing context */ char idxaff =3D sqlite3TableColumnAffinity(pTab->def, = iCol); enum affinity_type lhs_aff =3D = sqlite3ExprAffinity(pLhs); - char cmpaff =3D sql_affinity_result(lhs_aff, = idxaff); - testcase(cmpaff =3D=3D AFFINITY_BLOB); - testcase(cmpaff =3D=3D AFFINITY_TEXT); - switch (cmpaff) { - case AFFINITY_BLOB: - break; - case AFFINITY_TEXT: - /* sql_affinity_result() only returns = TEXT if one side or the - * other has no affinity and the other = side is TEXT. Hence, - * the only way for cmpaff to be TEXT is = for idxaff to be TEXT - * and for the term on the LHS of the IN = to have no affinity. - */ - assert(idxaff =3D=3D AFFINITY_TEXT); - break; - default: - affinity_ok =3D = sqlite3IsNumericAffinity(idxaff); - } + /* + * Index search is possible only if types + * of columns match. + */ + if (lhs_aff !=3D idxaff) + affinity_ok =3D 0; } =20 if (affinity_ok) { @@ -3148,25 +3137,6 @@ sqlite3ExprCodeIN(Parse * pParse, /* = Parsing and code generating context */ * of the RHS using the LHS as a probe. If found, the result is * true. */ + sqlite3VdbeAddOp4(v, OP_Affinity, rLhs, nVector, 0, zAff, = nVector); - if ((pExpr->flags & EP_xIsSelect) - && !pExpr->is_ephemeral) { - struct SrcList *src_list =3D pExpr->x.pSelect->pSrc; - assert(src_list->nSrc =3D=3D 1); - - struct Table *tab =3D src_list->a[0].pTab; - assert(tab !=3D NULL); - struct index *pk =3D space_index(tab->space, 0); - assert(pk !=3D NULL); - - uint32_t fieldno =3D pk->def->key_def->parts[0].fieldno; - enum affinity_type affinity =3D - tab->def->fields[fieldno].affinity; - if (pk->def->key_def->part_count =3D=3D 1 && - affinity =3D=3D AFFINITY_INTEGER && (int)fieldno < = nVector) { - int reg_pk =3D rLhs + (int)fieldno; - sqlite3VdbeAddOp2(v, OP_MustBeInt, reg_pk, = destIfFalse); - } - } diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua index d38907c37..6d751dc52 100755 --- a/test/sql-tap/in3.test.lua +++ b/test/sql-tap/in3.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test =3D require("sqltester") -test:plan(22) +test:plan(27) =20 --!./tcltestrunner.lua -- 2007 November 29 @@ -92,7 +92,7 @@ test:do_test( return exec_neph(" SELECT a FROM t1 WHERE a+0 IN (SELECT a FROM = t1); ") end, { -- - 0, 1, 3, 5 + 1, 1, 3, 5 -- }) =20 @@ -293,39 +293,39 @@ test:do_test( test:do_test( "in3-3.2", function() - -- No affinity is applied before comparing "x" and "a". = Therefore - -- the index can be used (the comparison is false, = text!=3Dnumber). + -- Both columns have type 'BLOB' so they are comparable. + -- Moreover, we can use index and avoid materializing + -- retults into ephemeral table. return exec_neph(" SELECT x IN (SELECT a FROM t1) FROM t2 ") end, { -- - 0, 0 + 0, 1 -- }) =20 -if false then test:do_test( "in3-3.3", function() - -- Logically, numeric affinity is applied to both sides before=20= - -- the comparison. Therefore it is possible to use index = t1_i2. + -- the comparison, but index can't be used. return exec_neph(" SELECT x IN (SELECT b FROM t1) FROM t2 ") end, { -- - 0, 1 + 1, 1 -- }) =20 ---[[test:do_test( +test:do_test( "in3-3.4", function() - -- No affinity is applied before the comparison takes place. = Making - -- it possible to use index t1_i3. + -- BLOB is compatible with TEXT, however index can't + -- be used since under the hood BLOB is SCALAR (which + -- can contain not only STRING values) and TEXT is STRING. return exec_neph(" SELECT x IN (SELECT c FROM t1) FROM t2 ") end, { -- - 0, 1 + 1, 1 -- - })]] + }) =20 test:do_test( "in3-3.5", @@ -362,7 +362,6 @@ test:do_test( 1, 1 -- }) -end ----------------------------------------------------------------------- -- -- Test using a multi-column index. @@ -395,7 +394,7 @@ test:do_test( return exec_neph(" SELECT 'text' IN (SELECT b FROM t3)") end, { -- - 0, 0 + 0, 1 -- })