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

["n.pettik" <korablev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4068 bytes --]

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 <mailto: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) >= 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 |=


[-- Attachment #2: Type: text/html, Size: 34067 bytes --]