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

[n.pettik]

>> Still, other DBs (as usual I checked - Postgres, Oracle and DB2) process this
>> query as well:
>> 
>> select 1 from t1 having min(a) > 0;
>> 
>> In other words, not only aggregate can appear in SELECT args,
>> but also constant literals. Semantically, it seems to be correct.
>> Moreover, quieres like
>> 
>> select date() from t1 having min(a) > 0;
>> 
>> are valid too. What I mean is SELECT arguments must return
>> single value for all rows in table (i.e. be single-group).
>> 
>> From the first sight, this problem is likely to be minor, but I guess
>> we should implement correct behaviour as far as we are dealing
>> with this issue right now.
> This cases are totally useless, not so trivial to implement (need to calculate
> expression in VDBE, e.g.
> SELECT (SELECT s1 FROM te40 LIMIT 1) FROM te40 HAVING min(s1) > 0
> is a valid construction for Postgress.
> Moreover, current checks are the part of resolveSelectStep that 
> 
> Asked Peter, does it really important.

I would like to notice that we’ve decided to allow literals and constant functions also
appear in select arg list.

From new diff:

diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 9a2d6ff4e..7c0d9b44e 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,44 @@ 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;
+}

What about TK_NULL? What about unary minus?
This examples now fail:

tarantool> SELECT -1 FROM te40 HAVING SUM(s1) > 0;
---
- error: 'SQL error: HAVING argument must appear in the GROUP BY clause or be used
    in an aggregate function’
…

tarantool> SELECT  NULL FROM te40 HAVING SUM(s1) > 0;
---
- error: 'SQL error: HAVING argument must appear in the GROUP BY clause or be used
    in an aggregate function'
...

+
+/**
+ * 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_constat_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)
+               func =  sqlite3FindFunction(parser->db, func_name, -2, 0);
+       if (func == NULL)
+               return false;
+       return func != NULL ?
+              (func->funcFlags & SQLITE_FUNC_CONSTANT) != 0 : false;
+}
+

Consider refactoring: at last return ‘func’ can’t be NULL - this check is performed one
line above.

@@ -1203,7 +1204,7 @@ sql_expr_is_literal(struct Expr *expr)
  * @retval false Otherwise.
  */
 static bool
-sql_expr_is_constat_func(struct Parse *parser, struct Expr *expr)
+sql_expr_is_constant_func(struct Parse *parser, struct Expr *expr)
 {
        if (expr->op != TK_FUNCTION)
                return false;
@@ -1212,12 +1213,17 @@ sql_expr_is_constat_func(struct Parse *parser, struct Expr *expr)
        int args_count = args_list != NULL ? args_list->nExpr : 0;
        struct FuncDef *func =
                sqlite3FindFunction(parser->db, func_name, args_count, 0);
-       if (func == NULL)
-               func =  sqlite3FindFunction(parser->db, func_name, -2, 0);
-       if (func == NULL)
-               return false;
-       return func != NULL ?
-              (func->funcFlags & SQLITE_FUNC_CONSTANT) != 0 : false;
+       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;
 }
 
 /*
@@ -1338,8 +1344,8 @@ resolveSelectStep(Walker * pWalker, Select * p)
                                return WRC_Abort;
                        is_all_select_agg &= (sNC.ncFlags & NC_HasAgg) != 0 ||
                                             sql_expr_is_literal(expr) ||
-                                            sql_expr_is_constat_func(pParse,
-                                                                     expr);
+                                            sql_expr_is_constant_func(pParse,
+                                                                      expr);