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 088F02DA6B for ; Mon, 3 Dec 2018 15:45:52 -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 VXL1Md2_OArO for ; Mon, 3 Dec 2018 15:45:51 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 B41F52DA41 for ; Mon, 3 Dec 2018 15:45:51 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: support HAVING without GROUP BY clause From: Vladislav Shpilevoy References: <2a59a404-d305-3be3-6c11-5be0d1a06363@tarantool.org> Message-ID: Date: Mon, 3 Dec 2018 23:45:49 +0300 MIME-Version: 1.0 In-Reply-To: <2a59a404-d305-3be3-6c11-5be0d1a06363@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Kirill Shcherbatov , tarantool-patches@freelists.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.