[tarantool-patches] Re: [PATCH] sql: remove support of partial indexes

[Alexander Turenko <alexander.turenko@tarantool.org>]

Hi, Gleb!

Please, consider the review below. It mostly about changes that are not
related to the issue you resolving. Try to make your commits atomic:
just change that you describe in the commit header, nothing more.

Please, regenerate and commit your parse.c.

WBR, Alexander Turenko.

On Thu, Mar 29, 2018 at 01:58:51PM +0300, Gleb wrote:
> ...

> diff --git a/test/sql-tap/analyze9.test.lua b/test/sql-tap/analyze9.test.lua
> index 4ce575e..ddf7a7c 100755
> --- a/test/sql-tap/analyze9.test.lua
> +++ b/test/sql-tap/analyze9.test.lua
> ...
> @@ -78,7 +78,6 @@ test:do_execsql_test(
>      1.3,
>      [[
>          SELECT "tbl","idx","neq","nlt","ndlt",msgpack_decode_sample("sample") FROM "_sql_stat4" where "idx" = 'T1';
> -
>      ]], {
>          -- <1.3>
>          'T1', 'T1', '1', '0', '0', '(0)', 'T1', 'T1', '1', '1', '1', '(1)', 
> @@ -209,7 +208,7 @@ test:do_execsql_test(
>          INSERT INTO t1 SELECT a+2,3,'three'||substr(c,4) FROM t1 WHERE c GLOB 'one-*';
>          INSERT INTO t1 SELECT a+3,4,'four'||substr(c,4) FROM t1 WHERE c GLOB 'one-*';
>          INSERT INTO t1 SELECT a+4,5,'five'||substr(c,4) FROM t1 WHERE c GLOB 'one-*';
> -        INSERT INTO t1 SELECT a+5,6,'six'||substr(c,4) FROM t1 WHERE c GLOB 'one-*';	
> +        INSERT INTO t1 SELECT a+5,6,'six'||substr(c,4) FROM t1 WHERE c GLOB 'one-*';    
>          CREATE INDEX t1b ON t1(b);
>          ANALYZE;
>          SELECT c FROM t1 WHERE b=3 AND a BETWEEN 30 AND 60;
> @@ -252,16 +251,12 @@ test:do_test(
>              INSERT INTO t1(id, c, b, a) VALUES(null, 200, 1, 'a');
>              INSERT INTO t1(id, c, b, a) VALUES(null, 200, 1, 'b');
>              INSERT INTO t1(id, c, b, a) VALUES(null, 200, 1, 'c');
> -
>              INSERT INTO t1(id, c, b, a) VALUES(null, 200, 2, 'e');
>              INSERT INTO t1(id, c, b, a) VALUES(null, 200, 2, 'f');
> -
>              INSERT INTO t1(id, c, b, a) VALUES(null, 201, 3, 'g');
>              INSERT INTO t1(id, c, b, a) VALUES(null, 201, 4, 'h');
> -
>              ANALYZE;
>              SELECT count(*) FROM "_sql_stat4";
> -
>          ]])
>          end, {
>              -- <4.1>

Trailing spaces and redundant changes.

> -test:do_execsql_test(
> -    17.6,
> -    [[
> -        EXPLAIN QUERY PLAN SELECT * FROM t1 WHERE d IS NOT NULL AND a=0 AND b=0 AND c=10;
> -    ]], {
> -        -- <17.6>
> -        0, 0, 0, "SEARCH TABLE T1 USING COVERING INDEX I2 (C=? AND D>?)"
> -        -- </17.6>
> -    })
> +--test:do_execsql_test(
> +--    17.6,
> +--    [[
> +--        EXPLAIN QUERY PLAN SELECT * FROM t1 WHERE d IS NOT NULL AND a=0 AND b=0 AND c=10;
> +--    ]], {
> +          -- <17.6>
> +--        0, 0, 0, "SEARCH TABLE T1 USING COVERING INDEX I2 (C=? AND D>?)"
> +          -- </17.6>
> +--    })

Please, comment entire block like so:

> -- ...
> --        -- <17.6>
> --        0, 0, 0, "SEARCH TABLE T1 USING COVERING INDEX I2 (C=? AND D>?)"
> --        -- </17.6>
> -- ...

> @@ -1308,7 +1303,6 @@ test:do_execsql_test(
>          WITH r(x) AS (SELECT 1 UNION ALL SELECT x+1 FROM r WHERE x<=100) 
>          INSERT INTO t3 SELECT CASE WHEN (x>45 AND x<96) THEN 'B' ELSE 'A' END,
>              x, CASE WHEN (x<51) THEN 'one' ELSE 'two' END, x FROM r;
> -
>          CREATE INDEX i3 ON t3(c);
>          CREATE INDEX i4 ON t3(d);
>          ANALYZE;
> @@ -1354,7 +1348,6 @@ test:do_execsql_test(
>          CREATE TABLE t4(a COLLATE "unicode_ci", b, c, d, e, f, PRIMARY KEY(c, b, a));
>          CREATE INDEX i41 ON t4(e);
>          CREATE INDEX i42 ON t4(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 t4 SELECT a, b, c, d, e, f FROM data;
> @@ -1601,18 +1594,13 @@ test:do_execsql_test(
>          CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT, x, y, z);
>          CREATE INDEX i1 ON t1(x, y);
>          CREATE INDEX i2 ON t1(z);
> -
> -
>          WITH cnt(y) AS (SELECT 0 UNION ALL SELECT y+1 FROM cnt WHERE y<99), 
>              letters(x) AS (SELECT 'A' UNION SELECT 'B' UNION SELECT 'C' UNION SELECT 'D') 
>                  INSERT INTO t1(id, x, y) SELECT null, x, y FROM letters, cnt;
> -
>          WITH letters(x) AS (SELECT 'A' UNION SELECT 'B' UNION SELECT 'C' UNION SELECT 'D') 
>              INSERT INTO t1(id, x, y) SELECT null, x, 70 FROM letters;
> -
>          WITH cnt(i) AS (SELECT 407 UNION ALL SELECT i+1 FROM cnt WHERE i<9999) 
>              INSERT INTO t1(id, x, y) SELECT i, i, i FROM cnt;
> -
>          UPDATE t1 SET z = (id / 95);
>          ANALYZE;
>      ]])

Redundant changes.

> diff --git a/test/sql-tap/autoindex4.test.lua b/test/sql-tap/autoindex4.test.lua
> index 45bae48..bdb0b98 100755
> --- a/test/sql-tap/autoindex4.test.lua
> +++ b/test/sql-tap/autoindex4.test.lua
> @@ -1,6 +1,6 @@
>  #!/usr/bin/env tarantool
>  test = require("sqltester")
> -test:plan(7)
> +test:plan(6)
>  
>  --!./tcltestrunner.lua
>  -- 2014-10-24
> @@ -27,7 +27,6 @@ test:do_execsql_test(
>          INSERT INTO t1 VALUES(123,'abc'),(234,'def'),(234,'ghi'),(345,'jkl');
>          CREATE TABLE t2(x,y, primary key(x,y));
>          INSERT INTO t2 VALUES(987,'zyx'),(654,'wvu'),(987,'rqp');
> -
>          SELECT *, '|' FROM t1, t2 WHERE a=234 AND x=987 ORDER BY +b;
>      ]], {
>          -- <autoindex4-1.0>
> @@ -95,7 +94,6 @@ test:do_execsql_test(
>          INSERT INTO Items VALUES('Item1','Parent');
>          INSERT INTO Items VALUES('Item2','Parent');
>          CREATE TABLE B(Name text primary key);
> -
>          SELECT Items.ItemName
>            FROM Items
>              LEFT JOIN A ON (A.Name = Items.ItemName and Items.ItemName = 'dummy')
> @@ -108,22 +106,21 @@ test:do_execsql_test(
>          -- </autoindex4-3.0>
>      })
>  

Redundant changes.

> diff --git a/test/sql-tap/fkey1.test.lua b/test/sql-tap/fkey1.test.lua
> index 8749e1f..ed40b81 100755
> --- a/test/sql-tap/fkey1.test.lua
> +++ b/test/sql-tap/fkey1.test.lua
> @@ -1,6 +1,7 @@
>  #!/usr/bin/env tarantool
>  test = require("sqltester")
> -test:plan(19)
> +test:plan(16)
> +
>  
>  -- This file implements regression tests for foreign keys.
>  
> @@ -123,7 +124,6 @@ test:do_execsql_test(
>          INSERT INTO "xx4"("xx5") VALUES('abc');
>          INSERT INTO "xx1"("xx2","xx3") VALUES('uvw','xyz');
>          SELECT 1, "xx5" FROM "xx4";
> -
>      ]], {
>          -- <fkey1-4.1>
>          1, 'abc'
> @@ -208,36 +208,36 @@ test:do_execsql_test(
>          -- </fkey1-5.6>
>      })

Extra empty line added after plan, one empty line removed after SELECT.
Don't see how it correspond to the issue you solving here. Is it some
kind of code style refactoring? Why it was not discussed before
implementing?

> diff --git a/test/sql-tap/gh-2165-remove-support-partial-indexes.test.lua b/test/sql-tap/gh-2165-remove-support-partial-indexes.test.lua
> new file mode 100755
> index 0000000..a4e274b
> --- /dev/null
> +++ b/test/sql-tap/gh-2165-remove-support-partial-indexes.test.lua
> @@ -0,0 +1,16 @@
> +#!/usr/bin/env tarantool
> +test = require("sqltester")
> +
> +test:plan(1)
> +
> +
> +test:do_catchsql_test(
> +    "partial-index-1",
> +    [[
> +        CREATE TABLE t1 (a INTEGER PRIMARY KEY, b INTEGER)
> +        CREATE UNIQUE INDEX i ON t1 (a) WHERE a = 3;
> +    ]], {
> +        1,"keyword \"CREATE\" is reserved"
> +    })
> +
> +test:finish_test()

It is good to separate tables elements using comma and space.

Suggest to leave a comment that the test intended to be deleted after
partial index implementing in #2626.

> diff --git a/test/sql-tap/index7.test.lua b/test/sql-tap/index7.test.lua
> index c8f56eb..97ac194 100755
> --- a/test/sql-tap/index7.test.lua
> +++ b/test/sql-tap/index7.test.lua
> @@ -1,6 +1,7 @@
>  #!/usr/bin/env tarantool
>  test = require("sqltester")
> -test:plan(5)
> +test:plan(1)
> +
>  

Extra newline added.