[tarantool-patches] Re: [PATCH] sql: Fixes op-codes' generation for skip-scan

[n.pettik]

Hello. Do not start commit subject with capital letter after ‘:’.

Also, I don’t 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)
> 
> 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));
> 
> 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’.

The limit for commit message is 72 chars…
Just reminding you.

> 
> With the patch:
> P2 is always set in 'Column' as a column number
> in 'native' order.
> 
> Closes #3350; Closes #2859

Just enumerate with comma:
Closes #xxxx, #xxxx

> —

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
> 
> 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 != 0);
> 		sqlite3VdbeJumpHere(v, j);
> 		for (j = 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’t check it,
since you haven’t pushed your branch.

> 					  regBase + j);
> 			testcase(pIdx->aiColumn[j] == 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=345;
> +        ]],
> +        {"abc",345,7,8,"def",345,9,10}

Consider formatting of curly braces.