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 204D629038 for ; Mon, 4 Mar 2019 07:14:06 -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 DZxWmJlfZFpe for ; Mon, 4 Mar 2019 07:14:05 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 77C3C28D3E for ; Mon, 4 Mar 2019 07:14:05 -0500 (EST) From: "n.pettik" Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_C1A1D5AD-ED75-47B7-A304-EB6C76B75403" Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: fix code generation for aggregate in HAVING clause Date: Mon, 4 Mar 2019 15:14:02 +0300 In-Reply-To: <1BCDA75B-2817-4A40-9F7D-40E7919BDD98@tarantool.org> References: <750fa247185a20047e0ebd3242768ec81f12ad9f.1550768589.git.korablev@tarantool.org> <1BCDA75B-2817-4A40-9F7D-40E7919BDD98@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: Vladislav Shpilevoy --Apple-Mail=_C1A1D5AD-ED75-47B7-A304-EB6C76B75403 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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. >>=20 >> 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. >=20 > Ok, slightly fixed commit message. >=20 >>> 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. >>=20 >> 2. Did you mean **not** satisfied? >=20 > Yep, thx: >=20 > sql: fix code generation for aggregate in HAVING clause >=20 > When we allowed using HAVING clause without GROUP BY (b40f2443a), = one > possible combination was forgotten to be tested: >=20 > SELECT 1 FROM te40 HAVING SUM(s1) < 0; > -- And SUM(s1) >=3D 0, i.e. HAVING condition is false. >=20 > 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. >=20 > Closes #3932 > Follow-up #2364 >=20 >>> 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) =3D=3D 0); >>> pGroupBy =3D p->pGroupBy; >>> - if (pGroupBy || (sNC.ncFlags & NC_HasAgg) !=3D 0) { >>> + if ((pGroupBy !=3D NULL || p->pHaving !=3D NULL) || >>=20 >> 3. Why do you need the braces around >> "pGroupBy !=3D NULL || p->pHaving !=3D NULL=E2=80=9D ? >=20 > Doesn=E2=80=99t matter much. Fixed: >=20 > 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) =3D=3D 0); > pGroupBy =3D p->pGroupBy; > - if ((pGroupBy !=3D NULL || p->pHaving !=3D NULL) || > + if (pGroupBy !=3D NULL || p->pHaving !=3D NULL || > (sNC.ncFlags & NC_HasAgg) !=3D 0) { > assert(NC_MinMaxAgg =3D=3D SF_MinMaxAgg); > p->selFlags |=3D --Apple-Mail=_C1A1D5AD-ED75-47B7-A304-EB6C76B75403 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hello,

Any = progress here?

On 25 Feb 2019, at 21:33, n.pettik <korablev@tarantool.org> wrote:
On 25 = Feb 2019, at 15:58, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> 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) >=3D 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) =3D=3D 0);
= = pGroupBy =3D p->pGroupBy;
- if = (pGroupBy || (sNC.ncFlags & NC_HasAgg) !=3D 0) {
+ if = ((pGroupBy !=3D NULL || p->pHaving !=3D NULL) ||

3. Why do you need the braces = around
"pGroupBy !=3D NULL || p->pHaving !=3D NULL=E2=80=9D= ?

Doesn=E2=80=99t 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)
          &nb= sp;    */
          &nb= sp;   assert((p->selFlags & SF_Aggregate) =3D=3D = 0);
          &nb= sp;   pGroupBy =3D p->pGroupBy;
- =             &n= bsp; if ((pGroupBy !=3D NULL || p->pHaving !=3D NULL) = ||
+ =             &n= bsp; if (pGroupBy !=3D NULL || p->pHaving !=3D NULL ||
          &nb= sp;       (sNC.ncFlags & = NC_HasAgg) !=3D 0) {
          &nb= sp;           asser= t(NC_MinMaxAgg =3D=3D SF_MinMaxAgg);
          &nb= sp;           p->= ;selFlags |=3D

= --Apple-Mail=_C1A1D5AD-ED75-47B7-A304-EB6C76B75403--