[tarantool-patches] Re: [PATCH v2 1/1] sql: support HAVING without GROUP BY clause
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Dec 3 23:45:49 MSK 2018
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.
More information about the Tarantool-patches
mailing list