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 0F49427164 for ; Mon, 4 Mar 2019 07:52:43 -0500 (EST) 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 MFxjAkLoJPoY for ; Mon, 4 Mar 2019 07:52:42 -0500 (EST) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 C082126E40 for ; Mon, 4 Mar 2019 07:52:42 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: fix code generation for aggregate in HAVING clause References: <750fa247185a20047e0ebd3242768ec81f12ad9f.1550768589.git.korablev@tarantool.org> <1BCDA75B-2817-4A40-9F7D-40E7919BDD98@tarantool.org> From: Vladislav Shpilevoy Message-ID: <3d47533f-cf5e-d5f9-dc60-0e9d8c37ae6b@tarantool.org> Date: Mon, 4 Mar 2019 15:52:40 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik" , tarantool-patches@freelists.org Hi! I remember about that patch. But now I review only 1-2 times a week. I am going to review your patch in a couple of days. On 04/03/2019 15:14, n.pettik wrote: > Hello, > > Any progress here? > >> On 25 Feb 2019, at 21:33, n.pettik > wrote: >>> On 25 Feb 2019, at 15:58, Vladislav Shpilevoy > wrote: >>> Thanks for the patch! See 3 comments below. >>> On 21/02/2019 21:01, Nikita Pettik wrote: >>>> When we allowed using HAVING clause without GROUP BY (b40f2443a), one >>>> possible combination was forgotten to be tested: >>>> SELECT 1 FROM te40 HAVING SUM(s1) < 0; >>>> In other words, resulting set contains no aggregates, but HAVING does >>>> contain. >>> >>> 1. We have these tests: select5-9.10, select5-9.11, select5-9.12. They all >>> have no aggregates in the result set, but have in HAVING. So that was not >>> a problem. Problem was that we forgot to test a false condition. >> >> Ok, slightly fixed commit message. >> >>>> In this case no byte-code related to aggregate execution is >>>> emitted at all. Hence, query above equals to simple SELECT 1; >>>> Unfortunately, result of such query is the same when condition under >>>> HAVING clause is satisfied. >>> >>> 2. Did you mean **not** satisfied? >> >> Yep, thx: >> >>   sql: fix code generation for aggregate in HAVING clause >> >>   When we allowed using HAVING clause without GROUP BY (b40f2443a), one >>   possible combination was forgotten to be tested: >> >>   SELECT 1 FROM te40 HAVING SUM(s1) < 0; >>   -- And SUM(s1) >= 0, i.e. HAVING condition is false. >> >>   In other words, resulting set contains no aggregates, but HAVING does >>   contain, but condition is false. In this case no byte-code related to >>   aggregate execution is emitted at all. Hence, query above equals to >>   simple SELECT 1; Unfortunately, result of such query is the same when >>   condition under HAVING clause is unsatisfied.  To fix this behaviour, it >>   is enough to indicate to byte-code generator that we should analyze >>   aggregates not only in ORDER BY clauses, but also in HAVING clause. >> >>   Closes #3932 >>   Follow-up #2364 >> >>>> To fix this behaviour, it is enough to >>>> indicate to byte-code generator that we should analyze aggregates not >>>> only in ORDER BY clauses, but also in HAVING clause. >>>> Closes #3932 >>>> Follow-up #2364 >>>> --- >>>> src/box/sql/resolve.c         | 10 +++++++--- >>>> test/sql-tap/select5.test.lua | 25 ++++++++++++++++++++++++- >>>> 2 files changed, 31 insertions(+), 4 deletions(-) >>>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c >>>> index bc208cc9d..e9a1b09f7 100644 >>>> --- a/src/box/sql/resolve.c >>>> +++ b/src/box/sql/resolve.c >>>> @@ -1290,12 +1290,16 @@ resolveSelectStep(Walker * pWalker, Select * p) >>>> return WRC_Abort; >>>> } >>>> -/* If there are no aggregate functions in the result-set, and no GROUP BY >>>> -* expression, do not allow aggregates in any of the other expressions. >>>> +/* >>>> +* If there are no aggregate functions in the >>>> +* result-set, and no GROUP BY or HAVING >>>> +* expression, do not allow aggregates in any >>>> +* of the other expressions. >>>> */ >>>> assert((p->selFlags & SF_Aggregate) == 0); >>>> pGroupBy = p->pGroupBy; >>>> -if (pGroupBy || (sNC.ncFlags & NC_HasAgg) != 0) { >>>> +if ((pGroupBy != NULL || p->pHaving != NULL) || >>> >>> 3. Why do you need the braces around >>> "pGroupBy != NULL || p->pHaving != NULL” ? >> >> Doesn’t matter much. Fixed: >> >> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c >> index e9a1b09f7..0184bc047 100644 >> --- a/src/box/sql/resolve.c >> +++ b/src/box/sql/resolve.c >> @@ -1298,7 +1298,7 @@ resolveSelectStep(Walker * pWalker, Select * p) >>                */ >>               assert((p->selFlags & SF_Aggregate) == 0); >>               pGroupBy = p->pGroupBy; >> -               if ((pGroupBy != NULL || p->pHaving != NULL) || >> +               if (pGroupBy != NULL || p->pHaving != NULL || >>                   (sNC.ncFlags & NC_HasAgg) != 0) { >>                       assert(NC_MinMaxAgg == SF_MinMaxAgg); >>                       p->selFlags |= >