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 95D1923789 for ; Fri, 27 Apr 2018 07:01:04 -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 a63e4W3_x7Jo for ; Fri, 27 Apr 2018 07:01:04 -0400 (EDT) Received: from smtp1.mail.ru (smtp1.mail.ru [94.100.179.111]) (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 4C40623782 for ; Fri, 27 Apr 2018 07:01:03 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: Fixes op-codes' generation for skip-scan From: "n.pettik" In-Reply-To: <20180427100613.94160-1-ivan.koptelov@tarantool.org> Date: Fri, 27 Apr 2018 14:01:01 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20180427100613.94160-1-ivan.koptelov@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: Ivan Koptelov Hello. Do not start commit subject with capital letter after =E2=80=98:=E2= =80=99. Also, I don=E2=80=99t see your branch at remote repository: you should push it before sending patch (in order to check Travis status, at least). Generally speaking, patch seems to be OK, but consider several minor comments concerning formalisation. > Currently we have skip-scan optimization working > the way described here: > (https://sqlite.org/optoverview.html#skipscan) >=20 > To understand the problem solved consider the example > with some skip-scan query and a table created like > CREATE TABLE t(a, b, c, PRIMARY KEY(c, b, a)); >=20 > Before the patch: op-codes realization of skip-scan > relied on the following work of 'Column' op-code: > If P1 is a number of cursor pointing to the table > then P2 is number of column in 'native' order > (0 for a, 1 for b, etc) > If P1 is a number of cursor pointing to the index > then P2 in number of column in 'index' order > (0 for c, 1 for b, etc) > But currently our 'Column' op-code always consider > P2 to be column number in 'native order=E2=80=99. The limit for commit message is 72 chars=E2=80=A6 Just reminding you. >=20 > With the patch: > P2 is always set in 'Column' as a column number > in 'native' order. >=20 > Closes #3350; Closes #2859 Just enumerate with comma: Closes #xxxx, #xxxx > =E2=80=94 Put here link to the branch and link to the issue. > src/box/sql/wherecode.c | 2 +- > test/sql-tap/skip-scan.test.lua | 102 = ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 103 insertions(+), 1 deletion(-) > create mode 100755 test/sql-tap/skip-scan.test.lua >=20 > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > index 6aec4ae79..a2620386c 100644 > --- a/src/box/sql/wherecode.c > +++ b/src/box/sql/wherecode.c > @@ -721,7 +721,7 @@ codeAllEqualityTerms(Parse * pParse, /* = Parsing context */ > VdbeCoverageIf(v, bRev !=3D 0); > sqlite3VdbeJumpHere(v, j); > for (j =3D 0; j < nSkip; j++) { > - sqlite3VdbeAddOp3(v, OP_Column, iIdxCur, j, > + sqlite3VdbeAddOp3(v, OP_Column, iIdxCur, = pIdx->aiColumn[j], Make sure that code fits into 80 chars. Now I can=E2=80=99t check it, since you haven=E2=80=99t pushed your branch. > regBase + j); > testcase(pIdx->aiColumn[j] =3D=3D XN_EXPR); > VdbeComment((v, "%s", = explainIndexColumnName(pIdx, j))); > diff --git a/test/sql-tap/skip-scan.test.lua = b/test/sql-tap/skip-scan.test.lua > new file mode 100755 > index 000000000..0c6edbf34 > --- /dev/null > +++ b/test/sql-tap/skip-scan.test.lua I would add to the name of test number of issue: gh-xxxx-skip-scan.test.lua Or, if it covers several issues, put comments before test cases indicating number of issue it belongs to. > +test:do_execsql_test( > + "skip-scan-1.4", > + [[ > + DROP TABLE IF EXISTS t1; > + CREATE TABLE t1(id INTEGER PRIMARY KEY, a TEXT, b INT, c = INT, d INT); > + CREATE INDEX t1abc ON t1(a,b,c); > + DROP TABLE IF EXISTS t2; > + CREATE TABLE t2(id PRIMARY KEY); > + INSERT INTO t2 VALUES(1); > + INSERT INTO t1 VALUES(1, 'abc',123,4,5); > + INSERT INTO t1 VALUES(2, 'abc',234,5,6); > + INSERT INTO t1 VALUES(3, 'abc',234,6,7); > + INSERT INTO t1 VALUES(4, 'abc',345,7,8); > + INSERT INTO t1 VALUES(5, 'def',567,8,9); > + INSERT INTO t1 VALUES(6, 'def',345,9,10); > + INSERT INTO t1 VALUES(7, 'bcd',100,6,11); > + ANALYZE; > + DELETE FROM "_sql_stat1"; > + DELETE FROM "_sql_stat4"; > + INSERT INTO "_sql_stat1" VALUES('t1','t1abc','10000 5000 = 2000 10'); > + ANALYZE t2; > + SELECT a,b,c,d FROM t1 WHERE b=3D345; > + ]], > + {"abc",345,7,8,"def",345,9,10} Consider formatting of curly braces.