Tarantool development patches archive
 help / color / mirror / Atom feed
From: "n.pettik" <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: support HAVING without GROUP BY clause
Date: Tue, 11 Sep 2018 00:43:26 +0300	[thread overview]
Message-ID: <00C60996-AB0B-4157-AF8D-DCD9B1F63D8D@tarantool.org> (raw)
In-Reply-To: <077750b5-7433-6122-5df9-caa6ff462e70@tarantool.org>

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.

> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 280ecd9..c38374d 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -600,6 +600,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);
> 		}
> @@ -1283,13 +1285,19 @@ resolveSelectStep(Walker * pWalker, Select * p)
> 		/* Set up the local name-context to pass to sqlite3ResolveExprNames() to
> 		 * resolve the result-set expression list.
> 		 */
> +		bool all_select_agg = true;

Nit: use ‘is_’ or ‘if_’ prefix for predicate variable.

> 		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;

Leave here explanation comment pls. Without any notion it is
quite hard to understand that this snippet helps to check
HAVING without GROUP BY queries.

> +		for (i = 0; i < p->pEList->nExpr; i++) {
> +			u16 has_agg_flag = sNC.ncFlags & NC_HasAgg;
> +			sNC.ncFlags &= ~NC_HasAgg;
> +			if (sqlite3ResolveExprNames(&sNC, p->pEList->a[i].pExpr))
> +				return WRC_Abort;
> +			all_select_agg &= (sNC.ncFlags & NC_HasAgg) != 0;
> +			sNC.ncFlags |= has_agg_flag;
> +		}

  reply	other threads:[~2018-09-10 21:43 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-28 16:39 [tarantool-patches] " Kirill Shcherbatov
2018-08-29 11:52 ` [tarantool-patches] " n.pettik
2018-08-29 13:29   ` Kirill Shcherbatov
2018-09-03 11:24     ` n.pettik
2018-09-03 13:23     ` n.pettik
2018-09-10  7:51       ` Kirill Shcherbatov
2018-09-10 21:43         ` n.pettik [this message]
2018-09-20 12:43           ` Kirill Shcherbatov
2018-10-01 16:37             ` n.pettik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=00C60996-AB0B-4157-AF8D-DCD9B1F63D8D@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v1 1/1] sql: support HAVING without GROUP BY clause' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox