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 0ECD6272B4 for ; Wed, 18 Jul 2018 08:34:55 -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 dyzvkIvHiPM5 for ; Wed, 18 Jul 2018 08:34:54 -0400 (EDT) Received: from mail-lj1-f193.google.com (mail-lj1-f193.google.com [209.85.208.193]) (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 91B1020FE5 for ; Wed, 18 Jul 2018 08:34:54 -0400 (EDT) Received: by mail-lj1-f193.google.com with SMTP id l15-v6so3955408lji.6 for ; Wed, 18 Jul 2018 05:34:54 -0700 (PDT) MIME-Version: 1.0 References: <1530533500-9666-1-git-send-email-hollow653@gmail.com> <20180717135933.cc2g7rlbb2xl4kgo@tkn_work_nb> In-Reply-To: <20180717135933.cc2g7rlbb2xl4kgo@tkn_work_nb> From: Nikita Tatunov Date: Wed, 18 Jul 2018 15:34:42 +0300 Message-ID: Subject: [tarantool-patches] Re: [PATCH] sql: assertion fail on nested select Content-Type: multipart/alternative; boundary="00000000000014a70b0571454ac7" 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: alexander.turenko@tarantool.org Cc: tarantool-patches@freelists.org --00000000000014a70b0571454ac7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hello, Alexander! Please consider changes to the patch: =D0=B2=D1=82, 17 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 16:59, Alexander T= urenko < alexander.turenko@tarantool.org>: > Hi Nikita! > > Please, consider comments below. > > WBR, Alexander Turenko. > > On Mon, Jul 02, 2018 at 03:11:40PM +0300, N.Tatunov wrote: > > To optimize the select-subquery tarantool uses > > subquery flattening function which is only used > > with respect to some restrictions. One of them > > was implemented improperly. > > > > Be certain: 'some restriction' and just 'improperly' are not good > problem descriptions. > > I would use wording like the following. > > > When a subquery is a compound select the subquery flattening > > optimization verify whether the ORDER BY clause presents, but only for > > the last subquery component. The commit fixes that to verify all > > subquery components. > > > In case of ORDER BY in non-last component of a compound select the > > error should be raised. Accepting of such subqueries for the > > flattening optimization prevents the error to be raised afterwards. > > Instead it stuck into assertion violation inside flattenSubquery (see > > the issue). > > Fixed: To optimize the select-subquery tarantool uses subquery flattening optimization which is only used with respect to some restrictions. When any of subselects but the last one has an ORDER BY clause we should raise an error. So subquery components containing it are not accepted to be optimized. Checking part was only checking if the last subselect contains the clause which led to an assertion fail. With the patch applied flattening is not used in case any of subselects has an ORDER BY. > I checked sqlite3 check-in 8000d230 (it is immediate predecessor of > c9104b59 where the flattening optimization seems to be changed, but > tarantool didn't updated with that check-in). It has the assert, but has > no a change like yours, and correctly shows the following error: > > Error: ORDER BY clause should come after UNION ALL not before > > Why it is so? > > > +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); > > All -> ALL > Also fixed. --00000000000014a70b0571454ac7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello, Alexander! Please consider changes to the patch:
=D0=B2=D1=82, 17 =D0=B8=D1= =8E=D0=BB. 2018 =D0=B3. =D0=B2 16:59, Alexander Turenko <alexander.turenko@tarantool.org>= :
Hi Nikita!

Please, consider comments below.

WBR, Alexander Turenko.

On Mon, Jul 02, 2018 at 03:11:40PM +0300, N.Tatunov wrote:
> To optimize the select-subquery tarantool uses
> subquery flattening function which is only used
> with respect to some restrictions. One of them
> was implemented improperly.
>

Be certain: 'some restriction' and just 'improperly' are no= t good
problem descriptions.

I would use wording like the following.

> When a subquery is a compound select the subquery flattening
> optimization verify whether the ORDER BY clause presents, but only for=
> the last subquery component. The commit fixes that to verify all
> subquery components.

> In case of ORDER BY in non-last component of a compound select the
> error should be raised. Accepting of such subqueries for the
> flattening optimization prevents the error to be raised afterwards. > Instead it stuck into assertion violation inside flattenSubquery (see<= br> > the issue).


Fixed:

To optimize the sel= ect-subquery tarantool uses
subquery flattening optimization whic= h is only used
with respect to some restrictions. When any of
subselects but the last one has an ORDER BY
clause we shou= ld raise an error. So subquery
components containing it are not a= ccepted
to be optimized.

Checking part w= as only checking if the last
subselect contains the clause which = led to an
assertion fail. With the patch applied
flatte= ning is not used in case any of
subselects has an ORDER BY.
=
=C2=A0
I checked sqlite3 check-in 8000d230 (it is immediate predecessor of
c9104b59 where the flattening optimization seems to be changed, but
tarantool didn't updated with that check-in). It has the assert, but ha= s
no a change like yours, and correctly shows the following error:

Error: ORDER BY clause should come after UNION ALL not before

Why it is so?

> +test:do_catchsql_test(
> +=C2=A0 =C2=A0 =C2=A0"subquery-9.3",
> +=C2=A0 =C2=A0 =C2=A0[[
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0SELECT * 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=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 SELECT * FROM table1);

All -> ALL

Also fixed.=C2=A0
--00000000000014a70b0571454ac7--