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 2225B27BAA for ; Thu, 19 Jul 2018 08:47:18 -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 JhMuAmERC8lF for ; Thu, 19 Jul 2018 08:47:18 -0400 (EDT) Received: from mail-lj1-f171.google.com (mail-lj1-f171.google.com [209.85.208.171]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9DCA427B6F for ; Thu, 19 Jul 2018 08:47:17 -0400 (EDT) Received: by mail-lj1-f171.google.com with SMTP id l15-v6so7284471lji.6 for ; Thu, 19 Jul 2018 05:47:17 -0700 (PDT) MIME-Version: 1.0 References: <1530533500-9666-1-git-send-email-hollow653@gmail.com> In-Reply-To: From: Nikita Tatunov Date: Thu, 19 Jul 2018 15:47:04 +0300 Message-ID: Subject: [tarantool-patches] Re: [PATCH] sql: assertion fail on nested select Content-Type: multipart/alternative; boundary="00000000000037d17305715994d0" 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: korablev@tarantool.org Cc: tarantool-patches@freelists.org --00000000000037d17305715994d0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable =D1=81=D1=80, 18 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 23:36, n.pettik : > > > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > > index 54f78a9..c035691 100644 > > --- a/src/box/sql/select.c > > +++ b/src/box/sql/select.c > > @@ -3893,14 +3893,14 @@ flattenSubquery(Parse * pParse, /= * > Parsing context */ > > * queries. > > */ > > if (pSub->pPrior) { > > - if (pSub->pOrderBy) { > > - return 0; /* Restriction 20 */ > > - } > > if (isAgg || (p->selFlags & SF_Distinct) !=3D 0 > > || pSrc->nSrc !=3D 1) { > > return 0; > > } > > for (pSub1 =3D pSub; pSub1; pSub1 =3D pSub1->pPrior) { > > + if (pSub1->pOrderBy) { > > + return 0; /* Restriction 20 */ > > + } > > According to our code style: > - use explicit !=3D NULL comparison; > - don=E2=80=99t put bracers around one-line if statement; > - put comment above the code to be commented; > > Fixed. > >diff --git a/test/sql-tap/subquery.test.lua > b/test/sql-tap/subquery.test.lua > >--- a/test/sql-tap/subquery.test.lua > >+++ b/test/sql-tap/subquery.test.lua > >+test:do_execsql_test( > >+ "subquery-9.2", > >+ [[ > >+ SELECT 'abc' FROM (SELECT * FROM table1 UNION ALL > >+ SELECT * FROM table1 UNION ALL > >+ SELECT * FROM table1 ORDER BY 1); > >+ ]], { > >+ -- > >+ 'abc','abc','abc','abc','abc','abc' > >+ -- > >+ }) > > Why did you use here SELECT =E2=80=98abc=E2=80=99 and in other tests - SE= LECT *? > It is nitpicking, but just tests look a little weird.. Yes, kind of weird. Why not though? Fixed, anyways. Diff: diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 54f78a9..1a6b034 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -3893,14 +3893,14 @@ flattenSubquery(Parse * pParse, /* Parsing context */ * queries. */ if (pSub->pPrior) { - if (pSub->pOrderBy) { - return 0; /* Restriction 20 */ - } if (isAgg || (p->selFlags & SF_Distinct) !=3D 0 || pSrc->nSrc !=3D 1) { return 0; } for (pSub1 =3D pSub; pSub1; pSub1 =3D pSub1->pPrior) { + /* Restriction 20 */ + if (pSub1->pOrderBy !=3D NULL) + return 0; testcase((pSub1-> selFlags & (SF_Distinct | SF_Aggregate)) =3D=3D SF_Distinct); diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lu= a index 06631c1..8fde62d 100755 --- a/test/sql-tap/subquery.test.lua +++ b/test/sql-tap/subquery.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test =3D require("sqltester") -test:plan(69) +test:plan(73) --!./tcltestrunner.lua -- 2005 January 19 @@ -926,6 +926,52 @@ test:do_execsql_test( -- }) +test:do_catchsql_test( + "subquery-9.0", + [[ + DROP TABLE IF EXISTS table1; + CREATE TABLE table1 (id VARCHAR(100) PRIMARY KEY); + INSERT INTO table1 VALUES ('abc'), ('abd'); + ]], { + -- + 0 + -- + }) + +test:do_catchsql_test( + "subquery-9.1", + [[ + SELECT * FROM (SELECT * FROM table1 UNION ALL + SELECT * FROM table1 ORDER BY 1 UNION ALL + SELECT * FROM table1); + ]], { + -- + 1, 'ORDER BY clause should come after UNION ALL not before' + -- + }) + +test:do_execsql_test( + "subquery-9.2", + [[ + SELECT * FROM (SELECT * FROM table1 UNION ALL + SELECT * FROM table1 UNION ALL + SELECT * FROM table1 ORDER BY 1); + ]], { + -- + 'abc', 'abc', 'abc', 'abd', 'abd', 'abd' + -- + }) +test:do_catchsql_test( + "subquery-9.3", + [[ + SELECT * FROM (SELECT * FROM table1 ORDER BY 1 UNION ALL + SELECT * FROM table1 UNION ALL + SELECT * FROM table1); + ]], { + -- + 1, 'ORDER BY clause should come after UNION ALL not before' + -- + }) test:finish_test() --00000000000037d17305715994d0 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=D1=81= =D1=80, 18 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 23:36, n.pettik <korablev@tarantool.org>:

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 54f78a9..c035691 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -3893,14 +3893,14 @@ flattenSubquery(Parse * pParse,=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Parsing context */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 * queries.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (pSub->pPrior) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (pSub->pOrderBy= ) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return 0;=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Restriction 20 */
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (isAgg || (p-= >selFlags & SF_Distinct) !=3D 0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0||= pSrc->nSrc !=3D 1) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0return 0;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for (pSub1 =3D p= Sub; pSub1; pSub1 =3D pSub1->pPrior) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (pSub1->pOrderBy) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;=C2=A0 =C2=A0 =C2=A0 =C2=A0/* R= estriction 20 */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0}

According to our code style:
- use explicit !=3D NULL comparison;
- don=E2=80=99t put bracers around one-line if statement;
- put comment above the code to be commented;


Fixed.
=C2=A0
>diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.tes= t.lua
>--- a/test/sql-tap/subquery.test.lua
>+++ b/test/sql-tap/subquery.test.lua
>+test:do_execsql_test(
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0"subquery-9.2",
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0[[
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0SELECT 'abc= ' FROM (SELECT * FROM table1 UNION ALL
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0SELECT * FROM table1= UNION ALL
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 SELECT * FROM table1 ORDER= BY 1);
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0]], {
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0-- <subquery= -9.2>
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0'abc',&= #39;abc','abc','abc','abc','abc'
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0-- <subquery= -9.2>
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0})

Why did you use here SELECT =E2=80=98abc=E2=80=99 and in other tests - SELE= CT *?
It is nitpicking, but just tests look a little weird..
Yes, kind of weird. Why not though?
Fixed, anyways.

Diff:

diff --git a/src/box/sql/select.c= b/src/box/sql/select.c
index 54f78a9..1a6b034 100644
-= -- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@= -3893,14 +3893,14 @@ flattenSubquery(Parse * pParse, /* Parsing context */
=C2=A0 * queries.
=C2=A0 */
=C2=A0 = if (pSub->pPrior) {
- = if (pSub->pOrderBy) {
- return 0; /* Restriction 20 */
- }
=C2=A0 if (isAgg || (p->selFlags & SF_Dis= tinct) !=3D 0
=C2=A0 =C2= =A0 =C2=A0 || pSrc->nSrc !=3D 1) {
=C2=A0 return 0;
=C2=A0 }
=C2=A0 for= (pSub1 =3D pSub; pSub1; pSub1 =3D pSub1->pPrior) {
+ /* Restriction 20 */
+ if (pSub1->pOrderBy !=3D NULL)
+ return 0;
=C2=A0 testcase((pSub1->
=C2=A0= =C2=A0 selFlags & (SF_Distin= ct | SF_Aggregate)) =3D=3D
=C2=A0= SF_Distinct);
diff --git a/test/sql-tap/subquery.test= .lua b/test/sql-tap/subquery.test.lua
index 06631c1..8fde62d 1007= 55
--- a/test/sql-tap/subquery.test.lua
+++ b/test/sql-= tap/subquery.test.lua
@@ -1,6 +1,6 @@
=C2=A0#!/usr/bin/= env tarantool
=C2=A0test =3D require("sqltester")
=
-test:plan(69)
+test:plan(73)
=C2=A0
=C2= =A0--!./tcltestrunner.lua
=C2=A0-- 2005 January 19
@@ -= 926,6 +926,52 @@ test:do_execsql_test(
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0-- </subquery-8.1>
=C2=A0 =C2=A0 =C2=A0})
=C2=A0
+test:do_catchsql_test(
+ "subquery-9.0",
+ [[
+ DROP TABLE IF EXISTS table1;
+ = CREATE TABLE table1 (id VARCHAR(100) PRIMARY KEY);
+ INSERT INTO table1 VALUES ('abc'= ;), ('abd');
+ ]],= {
+ -- <subquery-9.0&= gt;
+ 0
+ -- <subquery-9.0>
+ })
+
+test:do_catchs= ql_test(
+ "subquery-= 9.1",
+ [[
= + SELECT * FROM (SELECT * FROM tabl= e1 UNION ALL
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0SELECT * FROM= table1 ORDER BY 1 UNION ALL
+ = =C2=A0 =C2=A0 =C2=A0 =C2=A0SELECT * FROM table1);
+ ]], {
+ -- <subquery-9.1>
+ 1, 'ORDER BY clause should come after UNION ALL not be= fore'
+ -- <subque= ry-9.1>
+ })
= +
+test:do_execsql_test(
+ "subquery-9.2",
+ [[
+ SELECT = * FROM (SELECT * FROM table1 UNION ALL
+=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0SELECT * FROM table1 UNION ALL
+ =C2=A0 =C2=A0SELECT * FROM table1 ORDE= R BY 1);
+ ]], {
+ -- <subquery-9.2>
+ 'abc', 'abc', &= #39;abc', 'abd', 'abd', 'abd'
+ -- <subquery-9.2>
+ })
=C2=A0
+test:do_c= atchsql_test(
+ "subq= uery-9.3",
+ [[
=
+ SELECT * FROM (SELECT * FROM= table1 ORDER BY 1 UNION ALL
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0SELECT * FROM table1 UNION ALL
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0SELECT * FROM table1);
+ ]], {
+=C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0-- <subquery-9.3>
+=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01, 'ORDER BY clause should come a= fter UNION ALL not before'
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0-- <subquery-9.3>
+ })
=C2=A0
=C2=A0test:finish_test()

--00000000000037d17305715994d0--