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

[Vladislav Shpilevoy]

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 <korablev at tarantool.org <mailto:korablev at tarantool.org>> wrote:
>>> On 25 Feb 2019, at 15:58, Vladislav Shpilevoy <v.shpilevoy at tarantool.org <mailto: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 |=
>