[tarantool-patches] Re: [PATCH] sql: correct confusing message

Tue Mar 27 16:13:32 MSK 2018

Hello.

First of all, please use real name: it is hard to understand who
has sent patch.

Also, add link to the branch and link to the issue after ‘—‘:
after git format-patch you still can edit text file.
For example:

‘''
Closes #3236
---

Branch: xxx
Issue: xxx

src/box/sql/build.c                    | 13 ++++++——
‘''

Finally, before sending patch, make sure that there is no redundant changes
(i.e. non functional or/and has nothing in common with intentions of the patch).
You can do it with ‘git diff HEAD~1’ command or any GUI tool (such as gitk).

Consider comments below: fix them or argue in case you are not agree.
You can send patch v2, or reply on this message and provide fixed code.

> On 27 Mar 2018, at 14:49, Hollow <hollow653 at gmail.com> wrote:
> 
> Required to fix the following message:
> 
> "- error: expressions prohibited in PRIMARY KEY and UNIQUE constraints"
> 
> Currently this message appears when user tries to CREATE
> functional index but it doesn't seem clear what is actually wrong.

Provide some clarification: this message may appear when user
creates non-unique index — it is initial problem.
User also can create UNIQUE index, and then this message will be correct.

> Thus far the message was corrected to the one that explains
> what is wrong with expression in the command.
> 
> Closes #3236
> ---
> src/box/sql/build.c                    | 13 ++++++-----
> test/sql-tap/colname.test.lua          |  6 ++---
> test/sql/message-func-indexes.result   | 40 ++++++++++++++++++++++++++++++++++
> test/sql/message-func-indexes.test.lua | 17 +++++++++++++++
> 4 files changed, 67 insertions(+), 9 deletions(-)
> create mode 100644 test/sql/message-func-indexes.result
> create mode 100644 test/sql/message-func-indexes.test.lua
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index dd0f45c..8de9e62 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1,4 +1,4 @@
> -/*
> + /*
>  * Copyright 2010-2017, Tarantool AUTHORS, please see AUTHORS file.

Redundant diff.

>  *
>  * Redistribution and use in source and binary forms, with or
> @@ -987,7 +987,8 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
> 			    sqlite3ExprSkipCollate(pList->a[i].pExpr);
> 			assert(pCExpr != 0);
> 			if (pCExpr->op != TK_ID) {
> -				sqlite3ErrorMsg(pParse, "expressions prohibited in PRIMARY KEY");
> +				sqlite3ErrorMsg(pParse, "PRIMARY KEY can't be based on "
> +				"non-COLUMN entities”);

I guess previous message was OK. I see no reason to change it: they are almost identical.

> 				goto primary_key_exit;
> 			}
> 			const char *zCName = pCExpr->u.zToken;
> @@ -1802,7 +1803,7 @@ emitNewSysSpaceSequenceRecord(Parse *pParse, int space_id, const char reg_seq_id
> 
> 	/* 1. Space id  */
> 	sqlite3VdbeAddOp2(v, OP_SCopy, space_id, first_col + 1);
> -	
> +

Redundant diff.
Even if there is wrong indentation, it has nothing in common with this patch.
For example, you can fix it in separate patch/commit, if you want do it.

> 	/* 2. Sequence id  */
> 	sqlite3VdbeAddOp2(v, OP_IntCopy, reg_seq_id, first_col + 2);
> 
> @@ -2015,7 +2016,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
> 
> 			sqlite3OpenTable(pParse, iCursor, sys_space_sequence,
> 					 OP_OpenWrite);
> -			
> +

Redundant diff.

> 			reg_space_seq_record = emitNewSysSpaceSequenceRecord(
> 				pParse,
> 				iSpaceId,
> @@ -3065,8 +3066,8 @@ sqlite3CreateIndex(Parse * pParse,	/* All information about this parse */
> 		pCExpr = sqlite3ExprSkipCollate(pListItem->pExpr);
> 		if (pCExpr->op != TK_COLUMN) {
> 			sqlite3ErrorMsg(pParse,
> -					"expressions prohibited in PRIMARY KEY and "
> -					"UNIQUE constraints");
> +					"functional INDEXES aren't supported "
> +					"in the current version”);

I wouldn’t outline word ‘indexes’ with upper-case.
You can check out style of error messages in src/box/errcode.h
They must be identical for all parts of Tarantool.

> 			goto exit_create_index;
> 		} else {
> 			j = pCExpr->iColumn;
> diff --git a/test/sql-tap/colname.test.lua b/test/sql-tap/colname.test.lua
> index 71010da..b6e7fc9 100755
> --- a/test/sql-tap/colname.test.lua
> +++ b/test/sql-tap/colname.test.lua
> @@ -637,19 +637,19 @@ test:do_test(
> test:do_catchsql_test(
>     "colname-11.1",
>     [[ create table t1(a, b, c, primary key('A'))]],
> -    {1, "expressions prohibited in PRIMARY KEY"})
> +    {1, "PRIMARY KEY can't be based on non-COLUMN entities"})
> 
> test:do_catchsql_test(
>     "colname-11.2",
>     [[CREATE TABLE t1(a, b, c, d, e, 
>       PRIMARY KEY(a), UNIQUE('b' COLLATE "unicode_ci" DESC));]],
> -    {1, "/expressions prohibited in PRIMARY KEY/"})
> +    {1, "/functional INDEXES aren't supported in the current version/"})
> 
> test:execsql("create table table1(a primary key, b, c)")
> 
> test:do_catchsql_test(
>     "colname-11.3",
>     [[ CREATE INDEX t1c ON table1('c'); ]],
> -    {1, "/expressions prohibited in PRIMARY KEY/"})
> +    {1, "/functional INDEXES aren't supported in the current version/"})
> 
> test:finish_test()
> diff --git a/test/sql/message-func-indexes.result b/test/sql/message-func-indexes.result
> new file mode 100644
> index 0000000..30359df
> --- /dev/null
> +++ b/test/sql/message-func-indexes.result
> @@ -0,0 +1,40 @@
> +test_run = require('test_run').new()
> +---
> +...
> +-- Creating tables
> +box.sql.execute("CREATE TABLE t1(id PRIMARY KEY, a)")
> +---
> +...
> +box.sql.execute("CREATE TABLE t2(object INT PRIMARY KEY, price INT, count INT)")
> +---
> +...
> +-- Expressions that're supposed to create functional indexes are supposed to return certain message
> +box.sql.execute("CREATE INDEX i1 ON t1(a+1)")
> +---
> +- error: functional INDEXES aren't supported in the current version
> +...
> +box.sql.execute("CREATE INDEX i2 ON t1(a)")
> +---
> +...
> +box.sql.execute("CREATE INDEX i3 ON t2(price + 100)")
> +---
> +- error: functional INDEXES aren't supported in the current version
> +...
> +box.sql.execute("CREATE INDEX i4 ON t2(price)")
> +---
> +...
> +box.sql.execute("CREATE INDEX i5 ON t2(count + 1")
> +---
> +- error: 'near "1": syntax error’

I guess this test is not correct within this patch.

> +...
> +box.sql.execute("CREATE INDEX i6 ON t2(count + count)")
> +---
> +- error: functional INDEXES aren't supported in the current version
> +...
> +-- Cleaning up
> +box.sql.execute("DROP TABLE t1")
> +---
> +...
> +box.sql.execute("DROP TABLE t2") 
> +---
> +...
> diff --git a/test/sql/message-func-indexes.test.lua b/test/sql/message-func-indexes.test.lua
> new file mode 100644
> index 0000000..43dd30a
> --- /dev/null
> +++ b/test/sql/message-func-indexes.test.lua
> @@ -0,0 +1,17 @@
> +test_run = require('test_run').new()
> +
> +-- Creating tables
> +box.sql.execute("CREATE TABLE t1(id PRIMARY KEY, a)”)

We are going to introduce strict typing in SQL, so it would be great
if you specified type of columns (in order to avoid future refactoring tests).

> +box.sql.execute("CREATE TABLE t2(object INT PRIMARY KEY, price INT, count INT)")
> +
> +-- Expressions that're supposed to create functional indexes are supposed to return certain message

Please, fit comments in 80 chars (also, in source code they should fit into 66).
It would be better if you put dot at the end of sentence.

> +box.sql.execute("CREATE INDEX i1 ON t1(a+1)")
> +box.sql.execute("CREATE INDEX i2 ON t1(a)")
> +box.sql.execute("CREATE INDEX i3 ON t2(price + 100)")
> +box.sql.execute("CREATE INDEX i4 ON t2(price)")
> +box.sql.execute("CREATE INDEX i5 ON t2(count + 1")
> +box.sql.execute("CREATE INDEX i6 ON t2(count + count)")
> +
> +-- Cleaning up
> +box.sql.execute("DROP TABLE t1")
> +box.sql.execute("DROP TABLE t2") 