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

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

> On 29 Aug 2018, at 16:29, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
>> Please, make it behave the same way as other DBs
>> and resend patch.

Still your patch doesn’t handle some cases. Example:

Postgres:

drop table if exists t1 \\
create table t1(id int primary key, a int, b int) \\
insert into t1 values(1, -2,3), (4,5,6), (7,8,9) \\
SELECT SUM(a),a FROM t1 HAVING sum(a) > 0 \\

42803: column "t1.a" must appear in the GROUP BY clause or be used in an aggregate function

db2:

db2 => SELECT SUM(a), a from t1 having sum(a) > 0
SQL0119N  An expression starting with "A" specified in a SELECT clause, HAVING 
clause, or ORDER BY clause is not specified in the GROUP BY clause or it is in 
a SELECT clause, HAVING clause, or ORDER BY clause with a column function and 
no GROUP BY clause is specified.  SQLSTATE=42803

Oracle:

ORA-00937: not a single-group group function

Tarantool:

tarantool> SELECT SUM(a),a FROM t1 HAVING sum(a) > 0
---
- - [11, 8]
...

> 
> ====================================
> 
> Allowed to use HAVING without GROUP BY when both select list
> and having clause have aggregate function.
> 
> Closes 2364.
> 
> @TarantoolBot document
> Title: HAVING without GROUP BY clause
> A query with a having clause should also have a group by clause.

Nit: outline also here HAVING and GROUP BY with capital letters pls. 

> Another ability is to use aggregate function both in select list
> and having clause.

I would re-phrase it like:

However, it is still possible to drop GROUP BY clause in case both SELECT
and HAVING arguments are aggregate functions

> Example:
> SELECT SUM(s1) FROM te40 HAVING SUM(s1) > 0;
> ---
> src/box/sql/resolve.c         | 30 +++++++++++++++----
> test/sql-tap/count.test.lua   |  8 ++---
> test/sql-tap/select3.test.lua |  2 +-
> test/sql-tap/select5.test.lua | 68 ++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 97 insertions(+), 11 deletions(-)
> 
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 280ecd9..9d28089 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -1304,12 +1304,32 @@ resolveSelectStep(Walker * pWalker, Select * p)
> 			sNC.ncFlags &= ~NC_AllowAgg;
> 		}
> 
> -		/* If a HAVING clause is present, then there must be a GROUP BY clause.
> +		/*
> +		 * If a HAVING clause is present, then there must
> +		 * be a GROUP BY clause or aggregate function
> +		 * should be specified.
> 		 */
> -		if (p->pHaving && !pGroupBy) {
> -			sqlite3ErrorMsg(pParse,
> -					"a GROUP BY clause is required before HAVING");
> -			return WRC_Abort;
> +		if (p->pHaving != NULL && pGroupBy == NULL) {
> +			struct NameContext having_nc;
> +			memset(&having_nc, 0, sizeof(having_nc));
> +			having_nc.pParse = pParse;
> +			having_nc.ncFlags = NC_AllowAgg;
> +			having_nc.pSrcList = p->pSrc;
> +			if ((p->selFlags & SF_Aggregate) != 0 &&
> +				sqlite3ResolveExprNames(&having_nc,
> +							p->pHaving) != 0)
> +				return WRC_Abort;
> +			if ((having_nc.ncFlags & NC_HasAgg) == 0) {
> +				const char *err_msg =
> +					tt_sprintf("HAVING expression must “

Nit: HAVING is rather clause, than expression. Overall, this msg looks
a little bit weird. In Postgres it is like:

column "t1.a" must appear in the GROUP BY clause or be used in an aggregate function

So, not HAVING clause must appear, but HAVING argument.

> +						   "appear in the GROUP BY "
> +						   "clause or be used in an "
> +						   "aggregate function");
> +				diag_set(ClientError, ER_SQL, err_msg);
> +				pParse->nErr++;
> +				pParse->rc = SQL_TARANTOOL_ERROR;
> +				return WRC_Abort;
> +			}
> 		}
> 
> 		/* Add the output column list to the name-context before parsing the
> 
> 
> diff --git a/test/sql-tap/select5.test.lua b/test/sql-tap/select5.test.lua
> index 7ca25ae..db3f447 100755
> --- a/test/sql-tap/select5.test.lua
> +++ b/test/sql-tap/select5.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(32)
> +test:plan(38)
> 
> --!./tcltestrunner.lua
> -- 2001 September 15
> @@ -412,5 +412,71 @@ test:do_execsql_test(
>         -- </select5-8.8>
>     })
> 
> +--
> +-- gh-2364: Support HAVING without GROUP BY clause.
> +--
> +test:do_catchsql_test(
> +    "select6-1.1",
> +    [[
> +        CREATE TABLE te40 (s1 INT, s2 INT, PRIMARY KEY (s1,s2));
> +        INSERT INTO te40 VALUES (1,1);
> +        INSERT INTO te40 VALUES (2,2);
> +        SELECT s1 FROM te40 HAVING s1 = 1;
> +    ]], {
> +    -- <select6-1.1>
> +    1, "SQL error: HAVING expression must appear in the GROUP BY clause or be used in an aggregate function"
> +    -- <select6-1.1>
> +})
> +
> +test:do_catchsql_test(
> +    "select6-1.2",
> +    [[
> +        SELECT SUM(s1) FROM te40 HAVING s1 = 2;
> +    ]], {
> +    -- <select6-1.2>
> +    1, "SQL error: HAVING expression must appear in the GROUP BY clause or be used in an aggregate function"
> +    -- <select6-1.2>
> +})
> +
> +test:do_catchsql_test(
> +    "select6-1.3",
> +    [[
> +        SELECT s1 FROM te40 HAVING SUM(s1) = 2;
> +    ]], {
> +    -- <select6-1.3>
> +    1, "SQL error: HAVING expression must appear in the GROUP BY clause or be used in an aggregate function"
> +    -- <select6-1.3>
> +})
> +
> +test:do_execsql_test(
> +    "select6-1.4",
> +    [[
> +        SELECT SUM(s1) FROM te40 HAVING SUM(s1) > 0;
> +    ]], {
> +    -- <select6-1.4>
> +    3
> +    -- <select6-1.4>
> +})
> +
> +test:do_execsql_test(
> +    "select6-2.1”,

Why 2.1 and not 1.5? The same for tests below.