[tarantool-patches] [PATCH 0/2] Add collation to built-in funcs and fix HAVING clause with aggregate

[tarantool-patches] [PATCH 0/2] Add collation to built-in funcs and fix HAVING clause with aggregate

[Nikita Pettik]

Issue #3932 consists of two independent bugs. Current patch-set
provides fixes for both of them.

Branch: https://github.com/tarantool/tarantool/tree/np/gh-3932-function-collation
Issue: https://github.com/tarantool/tarantool/issues/3932

Nikita Pettik (2):
  sql: derive collation for built-in functions
  sql: fix code generation for aggregate in HAVING clause

 src/box/sql/analyze.c         |  6 +++---
 src/box/sql/expr.c            | 23 +++++++++++++++++++++++
 src/box/sql/func.c            | 22 +++++++++++-----------
 src/box/sql/resolve.c         | 10 +++++++---
 src/box/sql/sqlInt.h          | 31 +++++++++++++++++++++++--------
 test/sql-tap/select5.test.lua | 25 ++++++++++++++++++++++++-
 test/sql/collation.result     | 28 ++++++++++++++++++++++++++++
 test/sql/collation.test.lua   | 11 +++++++++++
 8 files changed, 130 insertions(+), 26 deletions(-)

-- 
2.15.1

[tarantool-patches] [PATCH 1/2] sql: derive collation for built-in functions

[Nikita Pettik]

Functions such as trim(), substr() etc should return result with
collation derived from their arguments. So, lets add flag indicating
that collation of first argument must be applied to function's result to
SQL function definition. Using this flag, we can derive appropriate
collation in sql_expr_coll().

Part of #3932
---
 src/box/sql/analyze.c       |  6 +++---
 src/box/sql/expr.c          | 23 +++++++++++++++++++++++
 src/box/sql/func.c          | 22 +++++++++++-----------
 src/box/sql/sqlInt.h        | 31 +++++++++++++++++++++++--------
 test/sql/collation.result   | 28 ++++++++++++++++++++++++++++
 test/sql/collation.test.lua | 11 +++++++++++
 6 files changed, 99 insertions(+), 22 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index e0f2724a5..82db23200 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -359,7 +359,7 @@ static const FuncDef statInitFuncdef = {
 	0,			/* xFinalize */
 	"stat_init",		/* zName */
 	{0},
-	0
+	0, false
 };
 
 /*
@@ -615,7 +615,7 @@ static const FuncDef statPushFuncdef = {
 	0,			/* xFinalize */
 	"stat_push",		/* zName */
 	{0},
-	0
+	0, false
 };
 
 #define STAT_GET_STAT1 0	/* "stat" column of stat1 table */
@@ -743,7 +743,7 @@ static const FuncDef statGetFuncdef = {
 	0,			/* xFinalize */
 	"stat_get",		/* zName */
 	{0},
-	0
+	0, false
 };
 
 static void
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 35145cef6..3963c627a 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -226,6 +226,29 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
 			}
 			break;
 		}
+		if (op == TK_FUNCTION) {
+			uint32_t arg_count = p->x.pList == NULL ? 0 :
+					     p->x.pList->nExpr;
+			struct FuncDef *func = sqlFindFunction(parse->db,
+							       p->u.zToken,
+							       arg_count, 0);
+			if (func == NULL)
+				break;
+			if (func->is_coll_derived) {
+				/*
+				 * Now we use quite straightforward
+				 * approach assuming that resulting
+				 * collation is derived from first
+				 * argument. It is true at least for
+				 * built-in functions: trim, upper,
+				 * lower, replace, substr.
+				 */
+				assert(func->ret_type == FIELD_TYPE_STRING);
+				p = p->x.pList->a->pExpr;
+				continue;
+			}
+			break;
+		}
 		if (p->flags & EP_Collate) {
 			if (p->pLeft && (p->pLeft->flags & EP_Collate) != 0) {
 				p = p->pLeft;
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 83e0d12a1..532b38e4f 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1727,12 +1727,12 @@ sqlRegisterBuiltinFunctions(void)
 			  FIELD_TYPE_INTEGER),
 		FUNCTION2(likely, 1, 0, 0, noopFunc, SQL_FUNC_UNLIKELY,
 			  FIELD_TYPE_INTEGER),
-		FUNCTION(ltrim, 1, 1, 0, trimFunc, FIELD_TYPE_STRING),
-		FUNCTION(ltrim, 2, 1, 0, trimFunc, FIELD_TYPE_STRING),
-		FUNCTION(rtrim, 1, 2, 0, trimFunc, FIELD_TYPE_STRING),
-		FUNCTION(rtrim, 2, 2, 0, trimFunc, FIELD_TYPE_STRING),
-		FUNCTION(trim, 1, 3, 0, trimFunc, FIELD_TYPE_STRING),
-		FUNCTION(trim, 2, 3, 0, trimFunc, FIELD_TYPE_STRING),
+		FUNCTION_COLL(ltrim, 1, 1, 0, trimFunc),
+		FUNCTION_COLL(ltrim, 2, 1, 0, trimFunc),
+		FUNCTION_COLL(rtrim, 1, 2, 0, trimFunc),
+		FUNCTION_COLL(rtrim, 2, 2, 0, trimFunc),
+		FUNCTION_COLL(trim, 1, 3, 0, trimFunc),
+		FUNCTION_COLL(trim, 2, 3, 0, trimFunc),
 		FUNCTION(min, -1, 0, 1, minmaxFunc, FIELD_TYPE_SCALAR),
 		FUNCTION(min, 0, 0, 1, 0, FIELD_TYPE_SCALAR),
 		AGGREGATE2(min, 1, 0, 1, minmaxStep, minMaxFinalize,
@@ -1754,8 +1754,8 @@ sqlRegisterBuiltinFunctions(void)
 		FUNCTION(round, 1, 0, 0, roundFunc, FIELD_TYPE_INTEGER),
 		FUNCTION(round, 2, 0, 0, roundFunc, FIELD_TYPE_INTEGER),
 #endif
-		FUNCTION(upper, 1, 0, 1, UpperICUFunc, FIELD_TYPE_STRING),
-		FUNCTION(lower, 1, 0, 1, LowerICUFunc, FIELD_TYPE_STRING),
+		FUNCTION_COLL(upper, 1, 0, 1, UpperICUFunc),
+		FUNCTION_COLL(lower, 1, 0, 1, LowerICUFunc),
 		FUNCTION(hex, 1, 0, 0, hexFunc, FIELD_TYPE_STRING),
 		FUNCTION2(ifnull, 2, 0, 0, noopFunc, SQL_FUNC_COALESCE,
 			  FIELD_TYPE_INTEGER),
@@ -1765,10 +1765,10 @@ sqlRegisterBuiltinFunctions(void)
 		FUNCTION(version, 0, 0, 0, sql_func_version, FIELD_TYPE_STRING),
 		FUNCTION(quote, 1, 0, 0, quoteFunc, FIELD_TYPE_STRING),
 		VFUNCTION(row_count, 0, 0, 0, sql_row_count, FIELD_TYPE_INTEGER),
-		FUNCTION(replace, 3, 0, 0, replaceFunc, FIELD_TYPE_STRING),
+		FUNCTION_COLL(replace, 3, 0, 0, replaceFunc),
 		FUNCTION(zeroblob, 1, 0, 0, zeroblobFunc, FIELD_TYPE_SCALAR),
-		FUNCTION(substr, 2, 0, 0, substrFunc, FIELD_TYPE_STRING),
-		FUNCTION(substr, 3, 0, 0, substrFunc, FIELD_TYPE_STRING),
+		FUNCTION_COLL(substr, 2, 0, 0, substrFunc),
+		FUNCTION_COLL(substr, 3, 0, 0, substrFunc),
 		AGGREGATE(sum, 1, 0, 0, sumStep, sumFinalize, FIELD_TYPE_NUMBER),
 		AGGREGATE(total, 1, 0, 0, sumStep, totalFinalize, FIELD_TYPE_NUMBER),
 		AGGREGATE(avg, 1, 0, 0, sumStep, avgFinalize, FIELD_TYPE_NUMBER),
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2830ab639..5fb7285d8 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -1633,6 +1633,13 @@ struct FuncDef {
 	} u;
 	/* Return type. */
 	enum field_type ret_type;
+	/**
+	 * If function returns string, it may require collation
+	 * to be applied on its result. For instance, result of
+	 * substr() built-in function must have the same collation
+	 * as its first argument.
+	 */
+	bool is_coll_derived;
 };
 
 /*
@@ -1693,6 +1700,11 @@ struct FuncDestructor {
  *     as the user-data (sql_user_data()) for the function. If
  *     argument bNC is true, then the sql_FUNC_NEEDCOLL flag is set.
  *
+ *   FUNCTION_COLL
+ *     Like FUNCTION except it assumes that function returns
+ *     STRING which collation should be derived from first
+ *     argument (trim, substr etc).
+ *
  *   VFUNCTION(zName, nArg, iArg, bNC, xFunc)
  *     Like FUNCTION except it omits the sql_FUNC_CONSTANT flag.
  *
@@ -1717,28 +1729,31 @@ struct FuncDestructor {
  */
 #define FUNCTION(zName, nArg, iArg, bNC, xFunc, type) \
   {nArg, SQL_FUNC_CONSTANT|(bNC*SQL_FUNC_NEEDCOLL), \
-   SQL_INT_TO_PTR(iArg), 0, xFunc, 0, #zName, {0}, type }
+   SQL_INT_TO_PTR(iArg), 0, xFunc, 0, #zName, {0}, type, false}
+#define FUNCTION_COLL(zName, nArg, iArg, bNC, xFunc) \
+  {nArg, SQL_FUNC_CONSTANT|(bNC*SQL_FUNC_NEEDCOLL), \
+   SQL_INT_TO_PTR(iArg), 0, xFunc, 0, #zName, {0}, FIELD_TYPE_STRING, true}
 #define VFUNCTION(zName, nArg, iArg, bNC, xFunc, type) \
   {nArg, (bNC*SQL_FUNC_NEEDCOLL), \
-   SQL_INT_TO_PTR(iArg), 0, xFunc, 0, #zName, {0}, type }
+   SQL_INT_TO_PTR(iArg), 0, xFunc, 0, #zName, {0}, type, false }
 #define DFUNCTION(zName, nArg, iArg, bNC, xFunc, type) \
   {nArg, SQL_FUNC_SLOCHNG|(bNC*SQL_FUNC_NEEDCOLL), \
-   SQL_INT_TO_PTR(iArg), 0, xFunc, 0, #zName, {0}, type }
+   SQL_INT_TO_PTR(iArg), 0, xFunc, 0, #zName, {0}, type, false }
 #define FUNCTION2(zName, nArg, iArg, bNC, xFunc, extraFlags, type) \
   {nArg,SQL_FUNC_CONSTANT|(bNC*SQL_FUNC_NEEDCOLL)|extraFlags,\
-   SQL_INT_TO_PTR(iArg), 0, xFunc, 0, #zName, {0}, type }
+   SQL_INT_TO_PTR(iArg), 0, xFunc, 0, #zName, {0}, type, false }
 #define STR_FUNCTION(zName, nArg, pArg, bNC, xFunc) \
   {nArg, SQL_FUNC_SLOCHNG|(bNC*SQL_FUNC_NEEDCOLL), \
-   pArg, 0, xFunc, 0, #zName, {SQL_AFF_STRING, {0}}}
+   pArg, 0, xFunc, 0, #zName, {SQL_AFF_STRING, {0}}, false}
 #define LIKEFUNC(zName, nArg, arg, flags, type) \
   {nArg, SQL_FUNC_CONSTANT|flags, \
-   (void *)(SQL_INT_TO_PTR(arg)), 0, likeFunc, 0, #zName, {0}, type }
+   (void *)(SQL_INT_TO_PTR(arg)), 0, likeFunc, 0, #zName, {0}, type, false }
 #define AGGREGATE(zName, nArg, arg, nc, xStep, xFinal, type) \
   {nArg, (nc*SQL_FUNC_NEEDCOLL), \
-   SQL_INT_TO_PTR(arg), 0, xStep,xFinal,#zName, {0}, type}
+   SQL_INT_TO_PTR(arg), 0, xStep,xFinal,#zName, {0}, type, false}
 #define AGGREGATE2(zName, nArg, arg, nc, xStep, xFinal, extraFlags, type) \
   {nArg, (nc*SQL_FUNC_NEEDCOLL)|extraFlags, \
-   SQL_INT_TO_PTR(arg), 0, xStep,xFinal,#zName, {0}, type}
+   SQL_INT_TO_PTR(arg), 0, xStep,xFinal,#zName, {0}, type, false}
 
 /*
  * All current savepoints are stored in a linked list starting at
diff --git a/test/sql/collation.result b/test/sql/collation.result
index 5721ef854..439dc51c1 100644
--- a/test/sql/collation.result
+++ b/test/sql/collation.result
@@ -325,3 +325,31 @@ box.sql.execute("DROP TABLE t1;")
 box.sql.execute("DROP TABLE t0;")
 ---
 ...
+-- gh-3932: make sure set build-in functions derive collation
+-- from their arguments.
+--
+box.sql.execute("CREATE TABLE jj (s1 INT PRIMARY KEY, s2 CHAR(3) COLLATE \"unicode_ci\");")
+---
+...
+box.sql.execute("INSERT INTO jj VALUES (1,'A'), (2,'a')")
+---
+...
+box.sql.execute("SELECT DISTINCT trim(s2) FROM jj;")
+---
+- - ['A']
+...
+box.sql.execute("INSERT INTO jj VALUES (3, 'aS'), (4, 'AS');")
+---
+...
+box.sql.execute("SELECT DISTINCT replace(s2, 'S', 's') FROM jj;")
+---
+- - ['A']
+  - ['as']
+...
+box.sql.execute("SELECT DISTINCT substr(s2, 1, 1) FROM jj;")
+---
+- - ['A']
+...
+box.space.JJ:drop()
+---
+...
diff --git a/test/sql/collation.test.lua b/test/sql/collation.test.lua
index 4c649a444..d4acbf9f1 100644
--- a/test/sql/collation.test.lua
+++ b/test/sql/collation.test.lua
@@ -126,3 +126,14 @@ box.sql.execute("SELECT * FROM t1;")
 box.sql.execute("SELECT s1 FROM t0;")
 box.sql.execute("DROP TABLE t1;")
 box.sql.execute("DROP TABLE t0;")
+
+-- gh-3932: make sure set build-in functions derive collation
+-- from their arguments.
+--
+box.sql.execute("CREATE TABLE jj (s1 INT PRIMARY KEY, s2 CHAR(3) COLLATE \"unicode_ci\");")
+box.sql.execute("INSERT INTO jj VALUES (1,'A'), (2,'a')")
+box.sql.execute("SELECT DISTINCT trim(s2) FROM jj;")
+box.sql.execute("INSERT INTO jj VALUES (3, 'aS'), (4, 'AS');")
+box.sql.execute("SELECT DISTINCT replace(s2, 'S', 's') FROM jj;")
+box.sql.execute("SELECT DISTINCT substr(s2, 1, 1) FROM jj;")
+box.space.JJ:drop()
-- 
2.15.1

[tarantool-patches] Re: [PATCH 1/2] sql: derive collation for built-in functions

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 21/02/2019 21:01, Nikita Pettik wrote:
> Functions such as trim(), substr() etc should return result with
> collation derived from their arguments. So, lets add flag indicating
> that collation of first argument must be applied to function's result to
> SQL function definition. Using this flag, we can derive appropriate
> collation in sql_expr_coll().
> 
> Part of #3932
> ---
>   src/box/sql/analyze.c       |  6 +++---
>   src/box/sql/expr.c          | 23 +++++++++++++++++++++++
>   src/box/sql/func.c          | 22 +++++++++++-----------
>   src/box/sql/sqlInt.h        | 31 +++++++++++++++++++++++--------
>   test/sql/collation.result   | 28 ++++++++++++++++++++++++++++
>   test/sql/collation.test.lua | 11 +++++++++++
>   6 files changed, 99 insertions(+), 22 deletions(-)
> 
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 2830ab639..5fb7285d8 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -1633,6 +1633,13 @@ struct FuncDef {
>   	} u;
>   	/* Return type. */
>   	enum field_type ret_type;
> +	/**
> +	 * If function returns string, it may require collation
> +	 * to be applied on its result. For instance, result of
> +	 * substr() built-in function must have the same collation
> +	 * as its first argument.
> +	 */
> +	bool is_coll_derived;
>   };

This way works only for builtin functions taking not a
bind parameter ('?'). For user-defined functions and for
bind parameters it does not fit. How can you determine
a function's result collation, if it is not builtin, and
does not depend on arguments?

Does SQL standard allow to define user functions without
a runtime defined collation? If SQL standard does not define
SQL functions at all, then what other vendors do with that
problem?

[tarantool-patches] Re: [PATCH 1/2] sql: derive collation for built-in functions

[n.pettik]

> On 25 Feb 2019, at 15:58, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> Hi! Thanks for the patch!
> On 21/02/2019 21:01, Nikita Pettik wrote:
>> Functions such as trim(), substr() etc should return result with
>> collation derived from their arguments. So, lets add flag indicating
>> that collation of first argument must be applied to function's result to
>> SQL function definition. Using this flag, we can derive appropriate
>> collation in sql_expr_coll().
>> Part of #3932
>> ---
>>  src/box/sql/analyze.c       |  6 +++---
>>  src/box/sql/expr.c          | 23 +++++++++++++++++++++++
>>  src/box/sql/func.c          | 22 +++++++++++-----------
>>  src/box/sql/sqlInt.h        | 31 +++++++++++++++++++++++--------
>>  test/sql/collation.result   | 28 ++++++++++++++++++++++++++++
>>  test/sql/collation.test.lua | 11 +++++++++++
>>  6 files changed, 99 insertions(+), 22 deletions(-)
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index 2830ab639..5fb7285d8 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -1633,6 +1633,13 @@ struct FuncDef {
>>  	} u;
>>  	/* Return type. */
>>  	enum field_type ret_type;
>> +	/**
>> +	 * If function returns string, it may require collation
>> +	 * to be applied on its result. For instance, result of
>> +	 * substr() built-in function must have the same collation
>> +	 * as its first argument.
>> +	 */
>> +	bool is_coll_derived;
>>  };
> 
> This way works only for builtin functions taking not a
> bind parameter ('?’).

AFAIK, we can’t pass binding value with collation,
we just have no means to do things like this:

cn:execute('select trim(?)', { ‘ABCD’, collation = “unicode_ci" })

On the other hand, we can do this:

cn:execute('select trim(? COLLATE “unicode_ci")', { ‘ABCD’})

> For user-defined functions and for
> bind parameters it does not fit. How can you determine
> a function's result collation, if it is not builtin, and
> does not depend on arguments?

In no way. We can extend signature of sql_create_function
and allow to pass collation to be applied to returning value.
But I am not sure that we should do this. Anyway, it wouldn't
help us with the initial issue: in our case collation is dependent
on one of arguments, so it *dynamically* changes. Hence, I
guess these problems are barely related.

Also, inlining comment from P.Gulutzan:
(https://github.com/tarantool/tarantool/issues/3932)

‘’'

It is true that user-defined functions will not know some things about 
what an SQL caller is passing. We don't promise that they will, so I
think it is okay that it is the caller's responsibility to make sure
relevant information is passed explicitly. A possible issue is that the
function cannot use the utf8 module for all possible collations, but
that is not an SQL issue.  

‘''

> Does SQL standard allow to define user functions without
> a runtime defined collation? If SQL standard does not define
> SQL functions at all, then what other vendors do with that
> problem?

There’s no such opportunity in ANSI, if I’m not mistaking.
Generally speaking, other vendors have procedural SQL.
And since PSQL is a part of SQL, there are no such problems:
collation is a part of string-like types.

[tarantool-patches] Re: [PATCH 1/2] sql: derive collation for built-in functions

[Vladislav Shpilevoy]

>> For user-defined functions and for
>> bind parameters it does not fit. How can you determine
>> a function's result collation, if it is not builtin, and
>> does not depend on arguments?
> 
> In no way. We can extend signature of sql_create_function
> and allow to pass collation to be applied to returning value.
> But I am not sure that we should do this. Anyway, it wouldn't
> help us with the initial issue: in our case collation is dependent
> on one of arguments, so it *dynamically* changes. Hence, I
> guess these problems are barely related.
> 
> Also, inlining comment from P.Gulutzan:
> (https://github.com/tarantool/tarantool/issues/3932)
> 
> ‘’'
> 
> It is true that user-defined functions will not know some things about
> what an SQL caller is passing. We don't promise that they will, so I
> think it is okay that it is the caller's responsibility to make sure
> relevant information is passed explicitly. A possible issue is that the
> function cannot use the utf8 module for all possible collations, but
> that is not an SQL issue.
> 
> ‘''

So do you mean that any string returned from a user defined function
always has no any collation?

I do not talk about Lua functions only. In future we are going to
introduce SQL functions.

> 
>> Does SQL standard allow to define user functions without
>> a runtime defined collation? If SQL standard does not define
>> SQL functions at all, then what other vendors do with that
>> problem?
> 
> There’s no such opportunity in ANSI, if I’m not mistaking.
> Generally speaking, other vendors have procedural SQL.
> And since PSQL is a part of SQL, there are no such problems:
> collation is a part of string-like types.
> 

It contradicts with your way of collation return. If PSQL states
that collation is a part of string-like type, then it is
possible to implement a function doing something like this:

     function my_func()
         if (rand() % 2 == 0) then
             return 'abc' collate 'unicode_ci';
         return 'abc' collate 'unicode';
     end;

(I do not know PSQL syntax - the code above is pseudo- ).

As I see, now we have no runtime defined collations at all -
all of them are known during compilation. Struct Mem, storing
function return value, even has no collation member. I guess,
we can not implement runtime collations now.

[tarantool-patches] Re: [PATCH 1/2] sql: derive collation for built-in functions

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/03/07 17:44]:
> It contradicts with your way of collation return. If PSQL states
> that collation is a part of string-like type, then it is
> possible to implement a function doing something like this:
> 
>     function my_func()
>         if (rand() % 2 == 0) then
>             return 'abc' collate 'unicode_ci';
>         return 'abc' collate 'unicode';
>     end;
> 
> (I do not know PSQL syntax - the code above is pseudo- ).

It would be impossible in SQL/PSM, since a function return value
is strictly typed, so there would be a type error or automatic
coercion to return type collation.

> As I see, now we have no runtime defined collations at all -
> all of them are known during compilation. Struct Mem, storing
> function return value, even has no collation member. I guess,
> we can not implement runtime collations now.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] [PATCH 2/2] sql: fix code generation for aggregate in HAVING clause

[Nikita Pettik]

When we allowed using HAVING clause without GROUP BY (b40f2443a), one
possible combination was forgotten to be tested:

SELECT 1 FROM te40 HAVING SUM(s1) < 0;

In other words, resulting set contains no aggregates, but HAVING does
contain. In this case no byte-code related to aggregate execution is
emitted at all. Hence, query above equals to simple SELECT 1;
Unfortunately, result of such query is the same when condition under
HAVING clause is satisfied.  To fix this behaviour, it is enough to
indicate to byte-code generator that we should analyze aggregates not
only in ORDER BY clauses, but also in HAVING clause.

Closes #3932
Follow-up #2364
---
 src/box/sql/resolve.c         | 10 +++++++---
 test/sql-tap/select5.test.lua | 25 ++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index bc208cc9d..e9a1b09f7 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -1290,12 +1290,16 @@ resolveSelectStep(Walker * pWalker, Select * p)
 				return WRC_Abort;
 		}
 
-		/* 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.
+		/*
+		 * If there are no aggregate functions in the
+		 * result-set, and no GROUP BY or HAVING
+		 * expression, do not allow aggregates in any
+		 * of the other expressions.
 		 */
 		assert((p->selFlags & SF_Aggregate) == 0);
 		pGroupBy = p->pGroupBy;
-		if (pGroupBy || (sNC.ncFlags & NC_HasAgg) != 0) {
+		if ((pGroupBy != NULL || p->pHaving != NULL) ||
+		    (sNC.ncFlags & NC_HasAgg) != 0) {
 			assert(NC_MinMaxAgg == SF_MinMaxAgg);
 			p->selFlags |=
 			    SF_Aggregate | (sNC.ncFlags & NC_MinMaxAgg);
diff --git a/test/sql-tap/select5.test.lua b/test/sql-tap/select5.test.lua
index 0d132dbf8..0e3efb5fa 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(44)
+test:plan(46)
 
 --!./tcltestrunner.lua
 -- 2001 September 15
@@ -538,5 +538,28 @@ test:do_execsql_test(
     -- </select5-9.12>
 })
 
+-- gh-3932: bytecode is not emmited if aggregate is placed only
+-- in HAVING clause.
+--
+test:do_execsql_test(
+    "select5-9.13",
+    [[
+        SELECT 1 FROM te40 HAVING SUM(s1) < 0;
+    ]], {
+    -- <select5-9.13>
+    -- </select5-9.13>
+})
+
+test:do_execsql_test(
+    "select5-9.13.2",
+    [[
+            CREATE TABLE jj (s1 INT, s2 CHAR(1), PRIMARY KEY(s1));
+            INSERT INTO jj VALUES(1, 'A'), (2, 'a');
+            SELECT 1 FROM jj HAVING avg(s2) = 1 AND avg(s2) = 0;
+    ]], {
+    -- <select5-9.13.2>
+    -- </select5-9.13.2>
+})
+
 test:finish_test()
 
-- 
2.15.1

[tarantool-patches] Re: [PATCH 2/2] sql: fix code generation for aggregate in HAVING clause

[Vladislav Shpilevoy]

Thanks for the patch! See 3 comments below.

On 21/02/2019 21:01, Nikita Pettik wrote:
> When we allowed using HAVING clause without GROUP BY (b40f2443a), one
> possible combination was forgotten to be tested:
> 
> SELECT 1 FROM te40 HAVING SUM(s1) < 0;
> 
> In other words, resulting set contains no aggregates, but HAVING does
> contain.

1. We have these tests: select5-9.10, select5-9.11, select5-9.12. They all
have no aggregates in the result set, but have in HAVING. So that was not
a problem. Problem was that we forgot to test a false condition.

> In this case no byte-code related to aggregate execution is
> emitted at all. Hence, query above equals to simple SELECT 1;
> Unfortunately, result of such query is the same when condition under
> HAVING clause is satisfied.

2. Did you mean **not** satisfied?

> To fix this behaviour, it is enough to
> indicate to byte-code generator that we should analyze aggregates not
> only in ORDER BY clauses, but also in HAVING clause.
> 
> Closes #3932
> Follow-up #2364
> ---
>   src/box/sql/resolve.c         | 10 +++++++---
>   test/sql-tap/select5.test.lua | 25 ++++++++++++++++++++++++-
>   2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index bc208cc9d..e9a1b09f7 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -1290,12 +1290,16 @@ resolveSelectStep(Walker * pWalker, Select * p)
>   				return WRC_Abort;
>   		}
>   
> -		/* 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.
> +		/*
> +		 * If there are no aggregate functions in the
> +		 * result-set, and no GROUP BY or HAVING
> +		 * expression, do not allow aggregates in any
> +		 * of the other expressions.
>   		 */
>   		assert((p->selFlags & SF_Aggregate) == 0);
>   		pGroupBy = p->pGroupBy;
> -		if (pGroupBy || (sNC.ncFlags & NC_HasAgg) != 0) {
> +		if ((pGroupBy != NULL || p->pHaving != NULL) ||

3. Why do you need the braces around
"pGroupBy != NULL || p->pHaving != NULL" ?

> +		    (sNC.ncFlags & NC_HasAgg) != 0) {
>   			assert(NC_MinMaxAgg == SF_MinMaxAgg);
>   			p->selFlags |=
>   			    SF_Aggregate | (sNC.ncFlags & NC_MinMaxAgg);

[tarantool-patches] Re: [PATCH 2/2] sql: fix code generation for aggregate in HAVING clause

[n.pettik]

> On 25 Feb 2019, at 15:58, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> Thanks for the patch! See 3 comments below.
> On 21/02/2019 21:01, Nikita Pettik wrote:
>> When we allowed using HAVING clause without GROUP BY (b40f2443a), one
>> possible combination was forgotten to be tested:
>> SELECT 1 FROM te40 HAVING SUM(s1) < 0;
>> In other words, resulting set contains no aggregates, but HAVING does
>> contain.
> 
> 1. We have these tests: select5-9.10, select5-9.11, select5-9.12. They all
> have no aggregates in the result set, but have in HAVING. So that was not
> a problem. Problem was that we forgot to test a false condition.

Ok, slightly fixed commit message.

>> In this case no byte-code related to aggregate execution is
>> emitted at all. Hence, query above equals to simple SELECT 1;
>> Unfortunately, result of such query is the same when condition under
>> HAVING clause is satisfied.
> 
> 2. Did you mean **not** satisfied?

Yep, thx:

   sql: fix code generation for aggregate in HAVING clause

   When we allowed using HAVING clause without GROUP BY (b40f2443a), one
   possible combination was forgotten to be tested:

   SELECT 1 FROM te40 HAVING SUM(s1) < 0;
   -- And SUM(s1) >= 0, i.e. HAVING condition is false.

   In other words, resulting set contains no aggregates, but HAVING does
   contain, but condition is false. In this case no byte-code related to
   aggregate execution is emitted at all. Hence, query above equals to
   simple SELECT 1; Unfortunately, result of such query is the same when
   condition under HAVING clause is unsatisfied.  To fix this behaviour, it
   is enough to indicate to byte-code generator that we should analyze
   aggregates not only in ORDER BY clauses, but also in HAVING clause.

   Closes #3932
   Follow-up #2364

>> To fix this behaviour, it is enough to
>> indicate to byte-code generator that we should analyze aggregates not
>> only in ORDER BY clauses, but also in HAVING clause.
>> Closes #3932
>> Follow-up #2364
>> ---
>> src/box/sql/resolve.c         | 10 +++++++---
>> test/sql-tap/select5.test.lua | 25 ++++++++++++++++++++++++-
>> 2 files changed, 31 insertions(+), 4 deletions(-)
>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index bc208cc9d..e9a1b09f7 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -1290,12 +1290,16 @@ resolveSelectStep(Walker * pWalker, Select * p)
>> 				return WRC_Abort;
>> 		}
>> -		/* 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.
>> +		/*
>> +		 * If there are no aggregate functions in the
>> +		 * result-set, and no GROUP BY or HAVING
>> +		 * expression, do not allow aggregates in any
>> +		 * of the other expressions.
>> 		 */
>> 		assert((p->selFlags & SF_Aggregate) == 0);
>> 		pGroupBy = p->pGroupBy;
>> -		if (pGroupBy || (sNC.ncFlags & NC_HasAgg) != 0) {
>> +		if ((pGroupBy != NULL || p->pHaving != NULL) ||
> 
> 3. Why do you need the braces around
> "pGroupBy != NULL || p->pHaving != NULL” ?

Doesn’t matter much. Fixed:

diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index e9a1b09f7..0184bc047 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -1298,7 +1298,7 @@ resolveSelectStep(Walker * pWalker, Select * p)
                */
               assert((p->selFlags & SF_Aggregate) == 0);
               pGroupBy = p->pGroupBy;
-               if ((pGroupBy != NULL || p->pHaving != NULL) ||
+               if (pGroupBy != NULL || p->pHaving != NULL ||
                   (sNC.ncFlags & NC_HasAgg) != 0) {
                       assert(NC_MinMaxAgg == SF_MinMaxAgg);
                       p->selFlags |=

[tarantool-patches] Re: [PATCH 2/2] sql: fix code generation for aggregate in HAVING clause

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 4068 bytes --]

Hello,

Any progress here?

> On 25 Feb 2019, at 21:33, n.pettik <korablev@tarantool.org> wrote:
>> On 25 Feb 2019, at 15:58, Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>> wrote:
>> Thanks for the patch! See 3 comments below.
>> On 21/02/2019 21:01, Nikita Pettik wrote:
>>> When we allowed using HAVING clause without GROUP BY (b40f2443a), one
>>> possible combination was forgotten to be tested:
>>> SELECT 1 FROM te40 HAVING SUM(s1) < 0;
>>> In other words, resulting set contains no aggregates, but HAVING does
>>> contain.
>> 
>> 1. We have these tests: select5-9.10, select5-9.11, select5-9.12. They all
>> have no aggregates in the result set, but have in HAVING. So that was not
>> a problem. Problem was that we forgot to test a false condition.
> 
> Ok, slightly fixed commit message.
> 
>>> In this case no byte-code related to aggregate execution is
>>> emitted at all. Hence, query above equals to simple SELECT 1;
>>> Unfortunately, result of such query is the same when condition under
>>> HAVING clause is satisfied.
>> 
>> 2. Did you mean **not** satisfied?
> 
> Yep, thx:
> 
>   sql: fix code generation for aggregate in HAVING clause
> 
>   When we allowed using HAVING clause without GROUP BY (b40f2443a), one
>   possible combination was forgotten to be tested:
> 
>   SELECT 1 FROM te40 HAVING SUM(s1) < 0;
>   -- And SUM(s1) >= 0, i.e. HAVING condition is false.
> 
>   In other words, resulting set contains no aggregates, but HAVING does
>   contain, but condition is false. In this case no byte-code related to
>   aggregate execution is emitted at all. Hence, query above equals to
>   simple SELECT 1; Unfortunately, result of such query is the same when
>   condition under HAVING clause is unsatisfied.  To fix this behaviour, it
>   is enough to indicate to byte-code generator that we should analyze
>   aggregates not only in ORDER BY clauses, but also in HAVING clause.
> 
>   Closes #3932
>   Follow-up #2364
> 
>>> To fix this behaviour, it is enough to
>>> indicate to byte-code generator that we should analyze aggregates not
>>> only in ORDER BY clauses, but also in HAVING clause.
>>> Closes #3932
>>> Follow-up #2364
>>> ---
>>> src/box/sql/resolve.c         | 10 +++++++---
>>> test/sql-tap/select5.test.lua | 25 ++++++++++++++++++++++++-
>>> 2 files changed, 31 insertions(+), 4 deletions(-)
>>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>>> index bc208cc9d..e9a1b09f7 100644
>>> --- a/src/box/sql/resolve.c
>>> +++ b/src/box/sql/resolve.c
>>> @@ -1290,12 +1290,16 @@ resolveSelectStep(Walker * pWalker, Select * p)
>>> 				return WRC_Abort;
>>> 		}
>>> -		/* 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.
>>> +		/*
>>> +		 * If there are no aggregate functions in the
>>> +		 * result-set, and no GROUP BY or HAVING
>>> +		 * expression, do not allow aggregates in any
>>> +		 * of the other expressions.
>>> 		 */
>>> 		assert((p->selFlags & SF_Aggregate) == 0);
>>> 		pGroupBy = p->pGroupBy;
>>> -		if (pGroupBy || (sNC.ncFlags & NC_HasAgg) != 0) {
>>> +		if ((pGroupBy != NULL || p->pHaving != NULL) ||
>> 
>> 3. Why do you need the braces around
>> "pGroupBy != NULL || p->pHaving != NULL” ?
> 
> Doesn’t matter much. Fixed:
> 
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index e9a1b09f7..0184bc047 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -1298,7 +1298,7 @@ resolveSelectStep(Walker * pWalker, Select * p)
>                */
>               assert((p->selFlags & SF_Aggregate) == 0);
>               pGroupBy = p->pGroupBy;
> -               if ((pGroupBy != NULL || p->pHaving != NULL) ||
> +               if (pGroupBy != NULL || p->pHaving != NULL ||
>                   (sNC.ncFlags & NC_HasAgg) != 0) {
>                       assert(NC_MinMaxAgg == SF_MinMaxAgg);
>                       p->selFlags |=


[-- Attachment #2: Type: text/html, Size: 34067 bytes --]

[tarantool-patches] Re: [PATCH 2/2] sql: fix code generation for aggregate in HAVING clause

[Vladislav Shpilevoy]

Hi! I remember about that patch. But now I review
only 1-2 times a week. I am going to review your patch
in a couple of days.

On 04/03/2019 15:14, n.pettik wrote:
> Hello,
> 
> Any progress here?
> 
>> On 25 Feb 2019, at 21:33, n.pettik <korablev@tarantool.org <mailto:korablev@tarantool.org>> wrote:
>>> On 25 Feb 2019, at 15:58, Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>> wrote:
>>> Thanks for the patch! See 3 comments below.
>>> On 21/02/2019 21:01, Nikita Pettik wrote:
>>>> When we allowed using HAVING clause without GROUP BY (b40f2443a), one
>>>> possible combination was forgotten to be tested:
>>>> SELECT 1 FROM te40 HAVING SUM(s1) < 0;
>>>> In other words, resulting set contains no aggregates, but HAVING does
>>>> contain.
>>>
>>> 1. We have these tests: select5-9.10, select5-9.11, select5-9.12. They all
>>> have no aggregates in the result set, but have in HAVING. So that was not
>>> a problem. Problem was that we forgot to test a false condition.
>>
>> Ok, slightly fixed commit message.
>>
>>>> In this case no byte-code related to aggregate execution is
>>>> emitted at all. Hence, query above equals to simple SELECT 1;
>>>> Unfortunately, result of such query is the same when condition under
>>>> HAVING clause is satisfied.
>>>
>>> 2. Did you mean **not** satisfied?
>>
>> Yep, thx:
>>
>>   sql: fix code generation for aggregate in HAVING clause
>>
>>   When we allowed using HAVING clause without GROUP BY (b40f2443a), one
>>   possible combination was forgotten to be tested:
>>
>>   SELECT 1 FROM te40 HAVING SUM(s1) < 0;
>>   -- And SUM(s1) >= 0, i.e. HAVING condition is false.
>>
>>   In other words, resulting set contains no aggregates, but HAVING does
>>   contain, but condition is false. In this case no byte-code related to
>>   aggregate execution is emitted at all. Hence, query above equals to
>>   simple SELECT 1; Unfortunately, result of such query is the same when
>>   condition under HAVING clause is unsatisfied.  To fix this behaviour, it
>>   is enough to indicate to byte-code generator that we should analyze
>>   aggregates not only in ORDER BY clauses, but also in HAVING clause.
>>
>>   Closes #3932
>>   Follow-up #2364
>>
>>>> To fix this behaviour, it is enough to
>>>> indicate to byte-code generator that we should analyze aggregates not
>>>> only in ORDER BY clauses, but also in HAVING clause.
>>>> Closes #3932
>>>> Follow-up #2364
>>>> ---
>>>> src/box/sql/resolve.c         | 10 +++++++---
>>>> test/sql-tap/select5.test.lua | 25 ++++++++++++++++++++++++-
>>>> 2 files changed, 31 insertions(+), 4 deletions(-)
>>>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>>>> index bc208cc9d..e9a1b09f7 100644
>>>> --- a/src/box/sql/resolve.c
>>>> +++ b/src/box/sql/resolve.c
>>>> @@ -1290,12 +1290,16 @@ resolveSelectStep(Walker * pWalker, Select * p)
>>>> return WRC_Abort;
>>>> }
>>>> -/* 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.
>>>> +/*
>>>> +* If there are no aggregate functions in the
>>>> +* result-set, and no GROUP BY or HAVING
>>>> +* expression, do not allow aggregates in any
>>>> +* of the other expressions.
>>>> */
>>>> assert((p->selFlags & SF_Aggregate) == 0);
>>>> pGroupBy = p->pGroupBy;
>>>> -if (pGroupBy || (sNC.ncFlags & NC_HasAgg) != 0) {
>>>> +if ((pGroupBy != NULL || p->pHaving != NULL) ||
>>>
>>> 3. Why do you need the braces around
>>> "pGroupBy != NULL || p->pHaving != NULL” ?
>>
>> Doesn’t matter much. Fixed:
>>
>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index e9a1b09f7..0184bc047 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -1298,7 +1298,7 @@ resolveSelectStep(Walker * pWalker, Select * p)
>>                */
>>               assert((p->selFlags & SF_Aggregate) == 0);
>>               pGroupBy = p->pGroupBy;
>> -               if ((pGroupBy != NULL || p->pHaving != NULL) ||
>> +               if (pGroupBy != NULL || p->pHaving != NULL ||
>>                   (sNC.ncFlags & NC_HasAgg) != 0) {
>>                       assert(NC_MinMaxAgg == SF_MinMaxAgg);
>>                       p->selFlags |=
> 

[tarantool-patches] Re: [PATCH 0/2] Add collation to built-in funcs and fix HAVING clause with aggregate

[Vladislav Shpilevoy]

LGTM.

On 21/02/2019 21:01, Nikita Pettik wrote:
> Issue #3932 consists of two independent bugs. Current patch-set
> provides fixes for both of them.
> 
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3932-function-collation
> Issue: https://github.com/tarantool/tarantool/issues/3932
> 
> Nikita Pettik (2):
>    sql: derive collation for built-in functions
>    sql: fix code generation for aggregate in HAVING clause
> 
>   src/box/sql/analyze.c         |  6 +++---
>   src/box/sql/expr.c            | 23 +++++++++++++++++++++++
>   src/box/sql/func.c            | 22 +++++++++++-----------
>   src/box/sql/resolve.c         | 10 +++++++---
>   src/box/sql/sqlInt.h          | 31 +++++++++++++++++++++++--------
>   test/sql-tap/select5.test.lua | 25 ++++++++++++++++++++++++-
>   test/sql/collation.result     | 28 ++++++++++++++++++++++++++++
>   test/sql/collation.test.lua   | 11 +++++++++++
>   8 files changed, 130 insertions(+), 26 deletions(-)
> 
> -- 
> 2.15.1
> 

[tarantool-patches] Re: [PATCH 0/2] Add collation to built-in funcs and fix HAVING clause with aggregate

[Kirill Yukhin]

Hello,

On 21 Feb 21:01, Nikita Pettik wrote:
> Issue #3932 consists of two independent bugs. Current patch-set
> provides fixes for both of them.
> 
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3932-function-collation
> Issue: https://github.com/tarantool/tarantool/issues/3932

I've rebased and checked your patches into 2.1 branch.

--
Regards, Kirill Yukhin