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 1A6F22A4B5 for ; Tue, 26 Mar 2019 10:14:27 -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 whD6t3Khrbym for ; Tue, 26 Mar 2019 10:14:26 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 A419E2A4C8 for ; Tue, 26 Mar 2019 10:14:25 -0400 (EDT) From: "n.pettik" Message-Id: <08DAB5A5-C5B9-4D0F-B3C3-D3B29505B2D0@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_9E6430B1-DA82-4990-8921-49373EAFC8DA" Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v4 7/8] sql: rework semantic errors Date: Tue, 26 Mar 2019 17:14:22 +0300 In-Reply-To: <20190322124822.GA7890@tarantool.org> References: <6dca562ff5163f5579f45d175b6cf188db52a09f.1552494059.git.imeevma@gmail.com> <53DBABF2-8730-4FF6-9C23-64BF3F9986D1@tarantool.org> <20190322124822.GA7890@tarantool.org> 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: tarantool-patches@freelists.org Cc: Imeev Mergen --Apple-Mail=_9E6430B1-DA82-4990-8921-49373EAFC8DA Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 >>> @@ -1042,27 +1044,34 @@ resolveCompoundOrderBy(Parse * pParse, = /* Parsing context. Leave error messages >>> * field) then convert that term into a copy of the corresponding = result set >>> * column. >>> * >>> - * If any errors are detected, add an error message to pParse and >>> - * return non-zero. Return zero if no errors are seen. >>> + * @param pParse Parsing context. >>> + * @param pSelect The SELECT statement containing the clause. >>> + * @param pOrderBy The ORDER BY or GROUP BY clause to be >>> + * processed. >>> + * @param is_order_by True if pOrderBy is ORDER BY, false if >>> + * pOrderBy is GROUP BY >>> + * @retval 0 On success, not 0 elsewhere. >>> */ >>> int >>> -sqlResolveOrderGroupBy(Parse * pParse, /* Parsing context. = Leave error messages here */ >>> - Select * pSelect, /* The SELECT statement = containing the clause */ >>> - ExprList * pOrderBy, /* The ORDER BY or GROUP = BY clause to be processed */ >>> - const char *zType /* "ORDER" or "GROUP" */ >>> - ) >>> +sqlResolveOrderGroupBy(Parse *pParse, Select *pSelect, ExprList = *pOrderBy, >>> + bool is_order_by) >>=20 >> Why did you decide to fix code style here? Anyway, you didn't = finished it >> (struct prefixes, param naming and so on and so forth) >>=20 > I fixed this because I thought that it is quite unreliable to > differentiate names of term using first letter of its name. Should > I remove these changes? But below you anyway get string representation. So, let=E2=80=99s return back previous version. >>> { >>> int i; >>> sql *db =3D pParse->db; >>> ExprList *pEList; >>> struct ExprList_item *pItem; >>> + const char *zType =3D is_order_by ? "ORDER" : "GROUP"; >>>=20 >>> if (pOrderBy =3D=3D 0 || pParse->db->mallocFailed) >>> return 0; >>>=20 >>> @@ -1096,22 +1105,23 @@ sqlResolveOrderGroupBy(Parse * pParse, = /* Parsing context. Leave error messages >>> * result-set expression. Otherwise, the expression is resolved in >>> * the usual way - using sqlResolveExprNames(). >>> * >>> - * This routine returns the number of errors. If errors occur, = then >>> - * an appropriate error message might be left in pParse. (OOM = errors >>> - * excepted.) >>> + * @param pNC The name context of the SELECT statement. >>> + * @param pSelect The SELECT statement containing the clause. >>> + * @param pOrderBy An ORDER BY or GROUP BY clause to resolve. >>> + * @param is_order_by True if pOrderBy is ORDER BY, false if >>> + * pOrderBy is GROUP BY >>> + * @retval 0 On success, not 0 elsewhere. >>> */ >>> static int >>> -resolveOrderGroupBy(NameContext * pNC, /* The name context of = the SELECT statement */ >>> - Select * pSelect, /* The SELECT statement holding = pOrderBy */ >>> - ExprList * pOrderBy, /* An ORDER BY or GROUP = BY clause to resolve */ >>> - const char *zType /* Either "ORDER" or "GROUP", as = appropriate */ >>> - ) >>=20 >> The same question. >>=20 > Answer above. >=20 > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 0022254..b6b6c24 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -603,7 +603,7 @@ sqlAddPrimaryKey(Parse * pParse, /* Parsing = context */ > goto primary_key_exit; > if (sql_space_primary_key(space) !=3D NULL) { > diag_set(ClientError, ER_CREATE_SPACE, space->def->name, > - "too many primary keys"); > + "primary key already exists=E2=80=9D); Sorry, could you fix message to =E2=80=9Cprimary key has been already = declared=E2=80=9D or =E2=80=9Ccan=E2=80=99t declare PRIMARY KEY more than once=E2=80=9D? = This error is related only to CREATE TABLE processing, so nothing is created at this stage = yet. Hence, error msg may seem a little bit misleading. > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index de0f282..efb895f 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -481,8 +481,8 @@ sqlRunParser(Parse * pParse, const char *zSql) > &pParse->sLastToken.isReserved); > i +=3D pParse->sLastToken.n; > if (i > mxSqlLen) { > - diag_set(ClientError, = ER_SQL_PARSER_GENERIC, > - "string or blob too big"); > + diag_set(ClientError, = ER_SQL_PARSER_LIMIT, > + "SQL command length", i, = mxSqlLen); > pParse->is_aborted =3D true; > break; > } >=20 > I will investigate this error a bit later. Ok, then file it (and the rest of comments which you haven=E2=80=99t = fixed). --Apple-Mail=_9E6430B1-DA82-4990-8921-49373EAFC8DA Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

@@ -1042,27 +1044,34 @@ = resolveCompoundOrderBy(Parse * pParse, /* Parsing context.  Leave = error messages
* field) then convert that term into a copy = of the corresponding result set
* column.
*- * If any errors are detected, add an error message to = pParse and
- * return non-zero.  Return zero if no = errors are seen.
+ * @param pParse Parsing context.
+ * @param pSelect The SELECT statement containing the = clause.
+ * @param pOrderBy The ORDER BY or GROUP BY = clause to be
+ * =             &n= bsp;   processed.
+ * @param is_order_by = True if pOrderBy is ORDER BY, false if
+ * =             &n= bsp;      pOrderBy is GROUP BY
+ * @retval 0 On success, not 0 elsewhere.
*/
int
-sqlResolveOrderGroupBy(Parse * = pParse, = /* Parsing context.  Leave error messages here */
- = = =    Select * = pSelect, = /* The SELECT statement containing the clause */
- = = =    ExprList * = pOrderBy, = /* The ORDER BY or GROUP BY clause to be processed */
- = = =    const char = *zType = /* "ORDER" or "GROUP" */
-    )
+sqlResolveOrderGroupBy(Parse *pParse, Select *pSelect, = ExprList *pOrderBy,
+       = ; bool is_order_by)

Why = did you decide to fix code style here? Anyway, you didn't finished it
(struct prefixes, param naming and so on and so forth)

I fixed this because I thought that it is quite unreliable = to
differentiate = names of term using first letter of its name. Should
I remove these = changes?

But = below you anyway get string representation. So, let=E2=80=99s = return
back previous version.

{
int i;
sql *db =3D= pParse->db;
ExprList *pEList;
= struct ExprList_item *pItem;
+ const = char *zType =3D is_order_by ? "ORDER" : "GROUP";

= if (pOrderBy =3D=3D 0 || pParse->db->mallocFailed)
= = return 0;

@@ -1096,22 +1105,23 @@ = sqlResolveOrderGroupBy(Parse * pParse, /* Parsing context.  Leave = error messages
* result-set expression.  Otherwise, = the expression is resolved in
* the usual way - using = sqlResolveExprNames().
*
- * This routine = returns the number of errors.  If errors occur, then
- = * an appropriate error message might be left in pParse.  (OOM = errors
- * excepted.)
+ * @param pNC The = name context of the SELECT statement.
+ * @param pSelect = The SELECT statement containing the clause.
+ * @param = pOrderBy An ORDER BY or GROUP BY clause to resolve.
+ * = @param is_order_by True if pOrderBy is ORDER BY, false if
+ = * =             &n= bsp;      pOrderBy is GROUP BY
+ * @retval 0 On success, not 0 elsewhere.
*/
static int
-resolveOrderGroupBy(NameContext * = pNC, = /* The name context of the SELECT statement */
-     Select * = pSelect, = /* The SELECT statement holding pOrderBy */
-     ExprList = * pOrderBy, = /* An ORDER BY or GROUP BY clause to resolve */
- = =     const = char *zType = /* Either "ORDER" or "GROUP", as appropriate */
- =    )

The same = question.

Answer above.

diff --git a/src/box/sql/build.c = b/src/box/sql/build.c
index 0022254..b6b6c24 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -603,7 +603,7 @@ sqlAddPrimaryKey(Parse * = pParse, /* Parsing context */
= goto = primary_key_exit;
= if = (sql_space_primary_key(space) !=3D NULL) {
= diag_set(ClientError, ER_CREATE_SPACE, = space->def->name,
- =  "too many primary = keys");
+ =  "primary key already = exists=E2=80=9D);

Sorry, could you fix message to =E2=80=9Cprimary = key has been already declared=E2=80=9D
or =E2=80=9Ccan=E2=80=99t= declare PRIMARY KEY more than once=E2=80=9D? This error is = related
only to CREATE TABLE processing, so nothing is created = at this stage yet.
Hence, error msg may seem a little bit = misleading.

diff --git a/src/box/sql/tokenize.c = b/src/box/sql/tokenize.c
index de0f282..efb895f 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -481,8 +481,8 @@ sqlRunParser(Parse * pParse, const char = *zSql)
=       = ;&pParse->sLastToken.isReserved);
= i +=3D pParse->sLastToken.n;
= if (i > mxSqlLen) {
- = diag_set(ClientError, ER_SQL_PARSER_GENERIC,
- =  "string or = blob too big");
+ = diag_set(ClientError, ER_SQL_PARSER_LIMIT,
+ =  "SQL = command length", i, mxSqlLen);
= = pParse->is_aborted =3D true;
= = break;
= }

I will = investigate this error a bit later.

Ok, then file it (and the rest of comments which = you haven=E2=80=99t fixed).


= --Apple-Mail=_9E6430B1-DA82-4990-8921-49373EAFC8DA--