[tarantool-patches] Re: [PATCH 2/2] sql: fix code generation for aggregate in HAVING clause

[n.pettik]

> On 25 Feb 2019, at 15:58, Vladislav Shpilevoy <v.shpilevoy at 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) >= 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 |=