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

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

[Ivan Koptelov]

From: IvanKoptelov <ivan.koptelov@tarantool.org>

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'.

With the patch:
P2 is always set in 'Column' as a column number
in 'native' order.

Closes #3350; Closes #2859
---
 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],
 					  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
@@ -0,0 +1,102 @@
+#!/usr/bin/env tarantool
+
+test = require("sqltester")
+test:plan(4)
+
+local function lindex(str, pos)
+    return str:sub(pos+1, pos+1)
+end
+
+local function int_to_char(i)
+    local res = ''
+    local char = 'abcdefghij'
+    local divs = {1000, 100, 10, 1}
+    for _, div in ipairs(divs) do
+        res = res .. lindex(char, math.floor(i/div) % 10)
+    end
+    return res
+end
+
+box.internal.sql_create_function("lindex", lindex)
+box.internal.sql_create_function("int_to_char", int_to_char)
+
+test:do_execsql_test(
+        "skip-scan-1.1",
+        [[
+            DROP TABLE IF EXISTS t1;
+            CREATE TABLE t1(a COLLATE "unicode_ci", b, c, d, e, f, PRIMARY KEY(c, b, a));
+            WITH data(a, b, c, d, e, f) AS
+            (SELECT int_to_char(0), 'xyz', 'zyx', '*', 0, 0 UNION ALL
+            SELECT int_to_char(f+1), b, c, d, (e+1) % 2, f+1 FROM data WHERE f<1024)
+            INSERT INTO t1 SELECT a, b, c, d, e, f FROM data;
+            ANALYZE;
+            SELECT COUNT(*) FROM t1 WHERE a < 'aaad';
+            DROP TABLE t1;
+        ]], {
+            3
+        })
+
+test:do_execsql_test(
+        "skip-scan-1.2",
+        [[
+            DROP TABLE IF EXISTS t2;
+            CREATE TABLE t2(a COLLATE "unicode_ci", b, c, d, e, f, PRIMARY KEY(e, f));
+            WITH data(a, b, c, d, e, f) AS
+            (SELECT int_to_char(0), 'xyz', 'zyx', '*', 0, 0 UNION ALL
+            SELECT int_to_char(f+1), b, c, d, (e+1) % 2, f+1 FROM data WHERE f<1024)
+            INSERT INTO t2 SELECT a, b, c, d, e, f FROM data;
+            ANALYZE;
+            SELECT COUNT(*) FROM t2 WHERE f < 500;
+            DROP TABLE t2;
+        ]], {
+            500
+        }
+)
+
+test:do_execsql_test(
+        "skip-scan-1.3",
+        [[
+            DROP TABLE IF EXISTS t3;
+            CREATE TABLE t3(a COLLATE "unicode_ci", b, c, d, e, f, PRIMARY KEY(a));
+            CREATE INDEX i31 ON t3(e, f);
+            WITH data(a, b, c, d, e, f) AS
+            (SELECT int_to_char(0), 'xyz', 'zyx', '*', 0, 0 UNION ALL
+            SELECT int_to_char(f+1), b, c, d, (e+1) % 2, f+1 FROM data WHERE f<1024)
+            INSERT INTO t3 SELECT a, b, c, d, e, f FROM data;
+            ANALYZE;
+            SELECT COUNT(*) FROM t3 WHERE f < 500;
+            DROP INDEX i31 on t3;
+            DROP TABLE t3;
+        ]], {
+            500
+        }
+)
+
+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}
+)
+
+
+test:finish_test()
-- 
2.14.3 (Apple Git-98)

[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.

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

[Ivan Koptelov]

[-- Attachment #1.1: Type: text/plain, Size: 963 bytes --]


>Hello. Do not start commit subject with capital letter after ‘:’.
Fixed.
>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). 
Pushed.  https://github.com/tarantool/tarantool/tree/sb/scip-scan-fix  
>The limit for commit message is 72 chars…
>Just reminding you. 
I am confused. 72 chars is a limit for the whole commit
message? 
>Just enumerate with comma:
>Closes #xxxx, #xxxx 
Ok.
>Put here link to the branch and link to the issue 
Ok.
>Make sure that code fits into 80 chars. Now I can’t check it,
>since you haven’t pushed your branch.
I am confused again. What is this 80-chars constraint about?
>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. 
Fxd.
>Consider formatting of curly braces.
Fxd.

-- 
Ivan Koptelov

[-- Attachment #1.2: Type: text/html, Size: 6080 bytes --]

[-- Attachment #2: 0001-sql-fixes-op-codes-generation-for-skip-scan.patch --]
[-- Type: application/x-patch, Size: 5600 bytes --]

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

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]


> I am confused. 72 chars is a limit for the whole commit
> message? 

72 for commit message body, 50 for commit subject.

> I am confused again. What is this 80-chars constraint about?

This is *acceptable* limit of symbols per line in source code.
Notice, that for comments in code it is 66 chars.
Your fix violates this rule: line is about 84 chars.

Also, we are going to support static types soon,
so lets add types for all tests in your suite.


[-- Attachment #2: Type: text/html, Size: 3160 bytes --]

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

[Ivan Koptelov]

[-- Attachment #1.1: Type: text/plain, Size: 326 bytes --]


>This is *acceptable* limit of symbols per line in source code.
>Notice, that for comments in code it is 66 chars. Your fix violates this rule: line is about 84 chars.
>
>Also, we are going to support static types soon,
>so lets add types for all tests in your suite. 
Fix code style violation. Add types.

-- 
Ivan Koptelov

[-- Attachment #1.2: Type: text/html, Size: 730 bytes --]

[-- Attachment #2: 0001-sql-fixes-op-codes-generation-for-skip-scan.patch --]
[-- Type: application/x-patch, Size: 5766 bytes --]

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

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 146 bytes --]


> Fix code style violation. Add types.
> 
> -- 
> Ivan Koptelov
> <0001-sql-fixes-op-codes-generation-for-skip-scan.patch>

Thanks. Now LGTM.




[-- Attachment #2: Type: text/html, Size: 2762 bytes --]

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

[Kirill Yukhin]

Hi Ivan, Nikita,
On 08 мая 19:59, n.pettik wrote:
> 
> > Fix code style violation. Add types.
> > 
> > -- 
> > Ivan Koptelov
> > <0001-sql-fixes-op-codes-generation-for-skip-scan.patch>
> 
> Thanks. Now LGTM.
I've checked the patch into 2.0

--
Regards, Kirill Yukhin