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 15E492B754 for ; Tue, 27 Mar 2018 09:13:36 -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 Ye1nacEPh6_O for ; Tue, 27 Mar 2018 09:13:36 -0400 (EDT) Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 6E5D52ABAE for ; Tue, 27 Mar 2018 09:13:35 -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: correct confusing message From: "n.pettik" In-Reply-To: <1522151344-28402-1-git-send-email-hollow653@gmail.com> Date: Tue, 27 Mar 2018 16:13:32 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <1522151344-28402-1-git-send-email-hollow653@gmail.com> 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: Hollow Cc: tarantool-patches@freelists.org 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 =E2=80=98=E2=80=94= =E2=80=98: after git format-patch you still can edit text file. For example: =E2=80=98'' Closes #3236 --- Branch: xxx Issue: xxx src/box/sql/build.c | 13 ++++++=E2=80=94=E2=80=94 =E2=80=98'' 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 =E2=80=98git diff HEAD~1=E2=80=99 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 wrote: >=20 > Required to fix the following message: >=20 > "- error: expressions prohibited in PRIMARY KEY and UNIQUE = constraints" >=20 > 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 =E2=80=94 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. >=20 > 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 >=20 > 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 !=3D 0); > if (pCExpr->op !=3D TK_ID) { > - sqlite3ErrorMsg(pParse, "expressions = prohibited in PRIMARY KEY"); > + sqlite3ErrorMsg(pParse, "PRIMARY KEY = can't be based on " > + "non-COLUMN entities=E2=80=9D); I guess previous message was OK. I see no reason to change it: they are = almost identical. > goto primary_key_exit; > } > const char *zCName =3D pCExpr->u.zToken; > @@ -1802,7 +1803,7 @@ emitNewSysSpaceSequenceRecord(Parse *pParse, int = space_id, const char reg_seq_id >=20 > /* 1. Space id */ > sqlite3VdbeAddOp2(v, OP_SCopy, space_id, first_col + 1); > -=09 > + 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); >=20 > @@ -2015,7 +2016,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context = */ >=20 > sqlite3OpenTable(pParse, iCursor, = sys_space_sequence, > OP_OpenWrite); > - =09 > + Redundant diff. > reg_space_seq_record =3D = emitNewSysSpaceSequenceRecord( > pParse, > iSpaceId, > @@ -3065,8 +3066,8 @@ sqlite3CreateIndex(Parse * pParse, /* All = information about this parse */ > pCExpr =3D sqlite3ExprSkipCollate(pListItem->pExpr); > if (pCExpr->op !=3D TK_COLUMN) { > sqlite3ErrorMsg(pParse, > - "expressions prohibited in = PRIMARY KEY and " > - "UNIQUE constraints"); > + "functional INDEXES aren't = supported " > + "in the current version=E2=80=9D);= I wouldn=E2=80=99t outline word =E2=80=98indexes=E2=80=99 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 =3D 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"}) >=20 > test:do_catchsql_test( > "colname-11.2", > [[CREATE TABLE t1(a, b, c, d, e,=20 > 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/"}) >=20 > test:execsql("create table table1(a primary key, b, c)") >=20 > 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/"}) >=20 > 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 =3D 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=E2=80=99 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")=20 > +--- > +... > 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 =3D require('test_run').new() > + > +-- Creating tables > +box.sql.execute("CREATE TABLE t1(id PRIMARY KEY, a)=E2=80=9D) 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")=20