[tarantool-patches] Re: [PATCH v2 1/1] sql: support HAVING without GROUP BY clause

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch! See 2 comments below.

> On 29/11/2018 17:33, Kirill Shcherbatov wrote:
>> Branch: http://github.com/tarantool/tarantool/tree/kshsh/gh-2364-having-without-groupby
>> Issue: https://github.com/tarantool/tarantool/issues/2364
>>
>> Allowed to make SELECT requests that have HAVING clause without
>> GROUP BY. It is possible when both - left and right parts of
>> request have aggregate function or constant value.
>>
>> Closes #2364.
>>
>> @TarantoolBot document
>> Title: HAVING without GROUP BY clause
>> A query with a having clause should also have a group by clause.
>> If you omit group by, all the rows not excluded by the where
>> clause return as a single group.
>> Because no grouping is performed between the where and having
>> clauses, they cannot act independently of each other. Having
>> acts like where because it affects the rows in a single group
>> rather than groups, except the having clause can still use
>> aggregates.
>> Having without group by is not supported for select from
>> multiple tables.

1. Please, add an example and a reference to a standard (I've
found more strict description in 2011 SQL standard
"Part 2: Foundation" on 403 page).

>> ---
>>   src/box/sql/resolve.c         |  97 +++++++++++++++++++++++---
>>   src/box/sql/sqliteInt.h       |   3 +-
>>   test/sql-tap/count.test.lua   |   8 +--
>>   test/sql-tap/select3.test.lua |   2 +-
>>   test/sql-tap/select5.test.lua | 128 +++++++++++++++++++++++++++++++++-
>>   5 files changed, 223 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index 9a2d6ff4e..34f0c836a 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -602,6 +602,8 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
>>           /* A lone identifier is the name of a column.
>>            */
>>       case TK_ID:{
>> +            if ((pNC->ncFlags & NC_AllowAgg) != 0)
>> +                pNC->ncFlags |= NC_HasUnaggregatedId;
>>               return lookupName(pParse, 0, pExpr->u.zToken, pNC,
>>                         pExpr);
>>           }
>> @@ -1180,6 +1182,50 @@ resolveOrderGroupBy(NameContext * pNC,    /* The name context of the SELECT stateme
>>       return sqlite3ResolveOrderGroupBy(pParse, pSelect, pOrderBy, zType);
>>   }
>> +/**
>> + * Test if specified expression is a regular literal.
>> + * @param expr Expression to analyse.
>> + * @retval true  If expression is literal.
>> + * @retval false Otherwise.
>> + */
>> +static bool
>> +sql_expr_is_literal(struct Expr *expr)
>> +{
>> +    return expr->op == TK_INTEGER || expr->op == TK_FLOAT ||
>> +           expr->op == TK_BLOB || expr->op == TK_STRING ||
>> +           expr->op == TK_NULL || expr->op == TK_UMINUS;
>> +}
>> +
>> +/**
>> + * Test if specified expression is a constant function.
>> + * @param parser Parsing context.
>> + * @param expr Expression to analyse.
>> + * @retval true  If expression is a existent constant function.
>> + * @retval false Otherwise.
>> + */
>> +static bool
>> +sql_expr_is_constant_func(struct Parse *parser, struct Expr *expr)
>> +{
>> +    if (expr->op != TK_FUNCTION)
>> +        return false;
>> +    char *func_name = expr->u.zToken;
>> +    struct ExprList *args_list = expr->x.pList;
>> +    int args_count = args_list != NULL ? args_list->nExpr : 0;
>> +    struct FuncDef *func =
>> +        sqlite3FindFunction(parser->db, func_name, args_count, 0);
>> +    if (func == NULL) {
>> +        /*
>> +         * If we fail to find function with exact number
>> +         * of arguments, lets try to search similar
>> +         * function but with different number of args.
>> +         */
>> +        func = sqlite3FindFunction(parser->db, func_name, -2, 0);
>> +        if (func == NULL)
>> +            return false;
>> +    }
>> +    return (func->funcFlags & SQLITE_FUNC_CONSTANT) != 0;
>> +}
>> +
>>   /*
>>    * Resolve names in the SELECT statement p and all of its descendants.
>>    */
>> @@ -1285,13 +1331,23 @@ resolveSelectStep(Walker * pWalker, Select * p)
>>           /* Set up the local name-context to pass to sqlite3ResolveExprNames() to
>>            * resolve the result-set expression list.
>>            */
>> +        bool is_all_select_agg = true;
>>           sNC.ncFlags = NC_AllowAgg;
>>           sNC.pSrcList = p->pSrc;
>>           sNC.pNext = pOuterNC;
>> -
>>           /* Resolve names in the result set. */
>> -        if (sqlite3ResolveExprListNames(&sNC, p->pEList))
>> -            return WRC_Abort;
>> +        for (i = 0; i < p->pEList->nExpr; i++) {
>> +            struct Expr *expr = p->pEList->a[i].pExpr;
>> +            u16 has_agg_flag = sNC.ncFlags & NC_HasAgg;
>> +            sNC.ncFlags &= ~NC_HasAgg;
>> +            if (sqlite3ResolveExprNames(&sNC, expr))
>> +                return WRC_Abort;
>> +            is_all_select_agg &= (sNC.ncFlags & NC_HasAgg) != 0 ||
>> +                         sql_expr_is_literal(expr) ||
>> +                         sql_expr_is_constant_func(pParse,
>> +                                       expr);
>> +            sNC.ncFlags |= has_agg_flag;
>> +        }
2. Why do you need sql_expr_is_literal() and sql_expr_is_constant_func()
if you have sqlite3ExprIsConstantOrFunction() ?

>>           /* 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.