Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions
@ 2021-09-10 16:01 Mergen Imeev via Tarantool-patches
  2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 01/13] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches
                   ` (12 more replies)
  0 siblings, 13 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:01 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch-set reworks aggregate functions. After this patch-set they should
work according to new rules. Non-aggregate SQL built-in functions will be
reworked in another patch-set.

https://github.com/tarantool/tarantool/issues/4145
https://github.com/tarantool/tarantool/tree/imeevma/gh-4145-aggregate-functions


Mergen Imeev (13):
  sql: use register P1 for number of arguments
  sql: remove AggStep0 and OP_BuiltinFunction0
  sql: move collation to struct sql_context
  sql: introduce mem_append()
  sql: remove sql_vdbemem_finalize()
  sql: rework SUM()
  sql: rework TOTAL()
  sql: rework AVG()
  sql: rework COUNT()
  sql: rework MIN() and MAX()
  sql: rework GROUP_CONCAT()
  sql: remove copying of result in finalizers
  sql: remove MEM_TYPE_AGG

 src/box/sql/date.c                       |  43 --
 src/box/sql/expr.c                       |  19 +-
 src/box/sql/func.c                       | 528 +++++++++--------------
 src/box/sql/mem.c                        |  75 +---
 src/box/sql/mem.h                        |  29 +-
 src/box/sql/select.c                     |  32 +-
 src/box/sql/sqlInt.h                     |  16 +-
 src/box/sql/vdbe.c                       | 178 ++------
 src/box/sql/vdbeInt.h                    |   6 +-
 src/box/sql/vdbeapi.c                    |  66 ---
 src/box/sql/vdbeaux.c                    |  11 +-
 test/sql-tap/built-in-functions.test.lua |  97 ++++-
 12 files changed, 419 insertions(+), 681 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 01/13] sql: use register P1 for number of arguments
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:01 ` Mergen Imeev via Tarantool-patches
  2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:01 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch makes OP_FunctionByName, OP_AggStep and OP_BuiltinFunction to
use register P1 for the number of arguments instead of register P5. This
makes it easier to use these opcodes.

Needed for #4145
---
 src/box/sql/expr.c    |  5 ++--
 src/box/sql/select.c  |  3 +-
 src/box/sql/vdbe.c    | 64 ++++++++++++++++++++-----------------------
 src/box/sql/vdbeInt.h |  1 -
 4 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index ee21c1ede..3b5df5292 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4107,18 +4107,17 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 						  (char *)coll, P4_COLLSEQ);
 			}
 			if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
-				sqlVdbeAddOp4(v, OP_BuiltinFunction0, constMask,
+				sqlVdbeAddOp4(v, OP_BuiltinFunction0, nFarg,
 					      r1, target, (char *)func,
 					      P4_FUNC);
 			} else {
-				sqlVdbeAddOp4(v, OP_FunctionByName, constMask,
+				sqlVdbeAddOp4(v, OP_FunctionByName, nFarg,
 					      r1, target,
 					      sqlDbStrNDup(pParse->db,
 							   func->def->name,
 							   func->def->name_len),
 					      P4_DYNAMIC);
 			}
-			sqlVdbeChangeP5(v, (u8) nFarg);
 			if (nFarg && constMask == 0) {
 				sqlReleaseTempRange(pParse, r1, nFarg);
 			}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 2fe38e319..92e40aef6 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -5639,9 +5639,8 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
 			sqlVdbeAddOp4(v, OP_CollSeq, regHit, 0, 0,
 					  (char *)coll, P4_COLLSEQ);
 		}
-		sqlVdbeAddOp3(v, OP_AggStep0, 0, regAgg, pF->iMem);
+		sqlVdbeAddOp3(v, OP_AggStep0, nArg, regAgg, pF->iMem);
 		sqlVdbeAppendP4(v, pF->func, P4_FUNC);
-		sqlVdbeChangeP5(v, (u8) nArg);
 		sql_expr_type_cache_change(pParse, regAgg, nArg);
 		sqlReleaseTempRange(pParse, regAgg, nArg);
 		if (addrNext) {
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 44533fb3e..a0f1be454 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1182,32 +1182,24 @@ case OP_CollSeq: {
 	break;
 }
 
-/* Opcode: BuiltinFunction0 P1 P2 P3 P4 P5
- * Synopsis: r[P3]=func(r[P2@P5])
+/* Opcode: BuiltinFunction0 P1 P2 P3 P4 *
+ * Synopsis: r[P3]=func(r[P2@P1])
  *
  * Invoke a user function (P4 is a pointer to a FuncDef object that
- * defines the function) with P5 arguments taken from register P2 and
+ * defines the function) with P1 arguments taken from register P2 and
  * successors.  The result of the function is stored in register P3.
  * Register P3 must not be one of the function inputs.
  *
- * P1 is a 32-bit bitmask indicating whether or not each argument to the
- * function was determined to be constant at compile time. If the first
- * argument was constant then bit 0 of P1 is set.
- *
  * See also: BuiltinFunction, AggStep, AggFinal
  */
-/* Opcode: BuiltinFunction P1 P2 P3 P4 P5
- * Synopsis: r[P3]=func(r[P2@P5])
+/* Opcode: BuiltinFunction P1 P2 P3 P4 *
+ * Synopsis: r[P3]=func(r[P2@P1])
  *
  * Invoke a user function (P4 is a pointer to an sql_context object that
- * contains a pointer to the function to be run) with P5 arguments taken
+ * contains a pointer to the function to be run) with P1 arguments taken
  * from register P2 and successors.  The result of the function is stored
  * in register P3.  Register P3 must not be one of the function inputs.
  *
- * P1 is a 32-bit bitmask indicating whether or not each argument to the
- * function was determined to be constant at compile time. If the first
- * argument was constant then bit 0 of P1 is set.
- *
  * SQL functions are initially coded as OP_BuiltinFunction0 with
  * P4 pointing to a FuncDef object.  But on first evaluation,
  * the P4 operand is automatically converted into an sql_context
@@ -1223,7 +1215,7 @@ case OP_BuiltinFunction0: {
 	sql_context *pCtx;
 
 	assert(pOp->p4type == P4_FUNC);
-	n = pOp->p5;
+	n = pOp->p1;
 	assert(pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor));
 	assert(n==0 || (pOp->p2>0 && pOp->p2+n<=(p->nMem+1 - p->nCursor)+1));
 	assert(pOp->p3<pOp->p2 || pOp->p3>=pOp->p2+n);
@@ -1233,7 +1225,6 @@ case OP_BuiltinFunction0: {
 	pCtx->func = pOp->p4.func;
 	pCtx->iOp = (int)(pOp - aOp);
 	pCtx->pVdbe = p;
-	pCtx->argc = n;
 	pOp->p4type = P4_FUNCCTX;
 	pOp->p4.pCtx = pCtx;
 	pOp->opcode = OP_BuiltinFunction;
@@ -1242,6 +1233,7 @@ case OP_BuiltinFunction0: {
 }
 case OP_BuiltinFunction: {
 	int i;
+	int argc = pOp->p1;
 	sql_context *pCtx;
 
 	assert(pOp->p4type==P4_FUNCCTX);
@@ -1255,11 +1247,12 @@ case OP_BuiltinFunction: {
 	pOut = vdbe_prepare_null_out(p, pOp->p3);
 	if (pCtx->pOut != pOut) {
 		pCtx->pOut = pOut;
-		for(i=pCtx->argc-1; i>=0; i--) pCtx->argv[i] = &aMem[pOp->p2+i];
+		for(i = 0; i < argc; ++i)
+			pCtx->argv[i] = &aMem[pOp->p2 + i];
 	}
 
 #ifdef SQL_DEBUG
-	for(i=0; i<pCtx->argc; i++) {
+	for(i = 0; i < argc; i++) {
 		assert(memIsValid(pCtx->argv[i]));
 		REGISTER_TRACE(p, pOp->p2+i, pCtx->argv[i]);
 	}
@@ -1267,7 +1260,7 @@ case OP_BuiltinFunction: {
 	pCtx->is_aborted = false;
 	assert(pCtx->func->def->language == FUNC_LANGUAGE_SQL_BUILTIN);
 	struct func_sql_builtin *func = (struct func_sql_builtin *)pCtx->func;
-	func->call(pCtx, pCtx->argc, pCtx->argv);
+	func->call(pCtx, argc, pCtx->argv);
 
 	/* If the function returned an error, throw an exception */
 	if (pCtx->is_aborted)
@@ -1283,11 +1276,11 @@ case OP_BuiltinFunction: {
 	break;
 }
 
-/* Opcode: FunctionByName * P2 P3 P4 P5
- * Synopsis: r[P3]=func(r[P2@P5])
+/* Opcode: FunctionByName P1 P2 P3 P4 *
+ * Synopsis: r[P3]=func(r[P2@P1])
  *
  * Invoke a user function (P4 is a pointer to a function object
- * that defines the function) with P5 arguments taken from
+ * that defines the function) with P1 arguments taken from
  * register P2 and successors. The result of the function is
  * stored in register P3.
  */
@@ -1303,7 +1296,7 @@ case OP_FunctionByName: {
 	 * turn out to be invalid after call.
 	 */
 	enum field_type returns = func->def->returns;
-	int argc = pOp->p5;
+	int argc = pOp->p1;
 	struct Mem *argv = &aMem[pOp->p2];
 	struct port args, ret;
 
@@ -4185,26 +4178,26 @@ case OP_DecrJumpZero: {      /* jump, in1 */
 }
 
 
-/* Opcode: AggStep0 * P2 P3 P4 P5
- * Synopsis: accum=r[P3] step(r[P2@P5])
+/* Opcode: AggStep0 P1 P2 P3 P4 *
+ * Synopsis: accum=r[P3] step(r[P2@P1])
  *
  * Execute the step function for an aggregate.  The
- * function has P5 arguments.   P4 is a pointer to the FuncDef
+ * function has P1 arguments.   P4 is a pointer to the FuncDef
  * structure that specifies the function.  Register P3 is the
  * accumulator.
  *
- * The P5 arguments are taken from register P2 and its
+ * The P1 arguments are taken from register P2 and its
  * successors.
  */
-/* Opcode: AggStep * P2 P3 P4 P5
- * Synopsis: accum=r[P3] step(r[P2@P5])
+/* Opcode: AggStep P1 P2 P3 P4 *
+ * Synopsis: accum=r[P3] step(r[P2@P1])
  *
  * Execute the step function for an aggregate.  The
- * function has P5 arguments.   P4 is a pointer to an sql_context
+ * function has P1 arguments.   P4 is a pointer to an sql_context
  * object that is used to run the function.  Register P3 is
  * as the accumulator.
  *
- * The P5 arguments are taken from register P2 and its
+ * The P1 arguments are taken from register P2 and its
  * successors.
  *
  * This opcode is initially coded as OP_AggStep0.  On first evaluation,
@@ -4228,7 +4221,6 @@ case OP_AggStep0: {
 	pCtx->func = pOp->p4.func;
 	pCtx->iOp = (int)(pOp - aOp);
 	pCtx->pVdbe = p;
-	pCtx->argc = n;
 	pOp->p4type = P4_FUNCCTX;
 	pOp->p4.pCtx = pCtx;
 	pOp->opcode = OP_AggStep;
@@ -4237,6 +4229,7 @@ case OP_AggStep0: {
 }
 case OP_AggStep: {
 	int i;
+	int argc = pOp->p1;
 	sql_context *pCtx;
 	Mem *pMem;
 	Mem t;
@@ -4252,11 +4245,12 @@ case OP_AggStep: {
 	 */
 	if (pCtx->pMem != pMem) {
 		pCtx->pMem = pMem;
-		for(i=pCtx->argc-1; i>=0; i--) pCtx->argv[i] = &aMem[pOp->p2+i];
+		for(i = 0; i < argc; ++i)
+			pCtx->argv[i] = &aMem[pOp->p2 + i];
 	}
 
 #ifdef SQL_DEBUG
-	for(i=0; i<pCtx->argc; i++) {
+	for(i = 0; i < argc; i++) {
 		assert(memIsValid(pCtx->argv[i]));
 		REGISTER_TRACE(p, pOp->p2+i, pCtx->argv[i]);
 	}
@@ -4269,7 +4263,7 @@ case OP_AggStep: {
 	pCtx->skipFlag = 0;
 	assert(pCtx->func->def->language == FUNC_LANGUAGE_SQL_BUILTIN);
 	struct func_sql_builtin *func = (struct func_sql_builtin *)pCtx->func;
-	func->call(pCtx, pCtx->argc, pCtx->argv);
+	func->call(pCtx, argc, pCtx->argv);
 	if (pCtx->is_aborted) {
 		mem_destroy(&t);
 		goto abort_due_to_error;
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index cfe743b94..575ab3f3d 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -182,7 +182,6 @@ struct sql_context {
 	 */
 	bool is_aborted;
 	u8 skipFlag;		/* Skip accumulator loading if true */
-	u8 argc;		/* Number of arguments */
 	sql_value *argv[1];	/* Argument set */
 };
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
  2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 01/13] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:01 ` Mergen Imeev via Tarantool-patches
  2021-09-10 16:27   ` Mergen Imeev via Tarantool-patches
  2021-09-14 21:21   ` Vladislav Shpilevoy via Tarantool-patches
  2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 03/13] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches
                   ` (10 subsequent siblings)
  12 siblings, 2 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:01 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch moves the initialization of sql_context out of the VDBE. This
allows us to remove the opcodes OP_BuiltinFunction0 and OP_AggStep0,
which work in a rather strange way. Moreover, due to the changes these
opcodes make to the VDBEs, it is possible that the estimated size of the
VDBE may change during execution of VDBE, which could lead to various
problems.

Needed for #4145
---
 src/box/sql/expr.c    | 12 +++++--
 src/box/sql/select.c  | 27 +++++++++++++--
 src/box/sql/sqlInt.h  |  3 ++
 src/box/sql/vdbe.c    | 81 ++-----------------------------------------
 src/box/sql/vdbeaux.c |  2 +-
 5 files changed, 41 insertions(+), 84 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 3b5df5292..6446ef091 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4107,9 +4107,15 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 						  (char *)coll, P4_COLLSEQ);
 			}
 			if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
-				sqlVdbeAddOp4(v, OP_BuiltinFunction0, nFarg,
-					      r1, target, (char *)func,
-					      P4_FUNC);
+				struct sql_context *ctx =
+					sql_context_new(v, func, nFarg);
+				if (ctx == NULL) {
+					pParse->is_aborted = true;
+					return -1;
+				}
+				sqlVdbeAddOp4(v, OP_BuiltinFunction, nFarg,
+					      r1, target, (char *)ctx,
+					      P4_FUNCCTX);
 			} else {
 				sqlVdbeAddOp4(v, OP_FunctionByName, nFarg,
 					      r1, target,
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 92e40aef6..e27dcb336 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -5639,8 +5639,13 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
 			sqlVdbeAddOp4(v, OP_CollSeq, regHit, 0, 0,
 					  (char *)coll, P4_COLLSEQ);
 		}
-		sqlVdbeAddOp3(v, OP_AggStep0, nArg, regAgg, pF->iMem);
-		sqlVdbeAppendP4(v, pF->func, P4_FUNC);
+		struct sql_context *ctx = sql_context_new(v, pF->func, nArg);
+		if (ctx == NULL) {
+			pParse->is_aborted = true;
+			return;
+		}
+		sqlVdbeAddOp3(v, OP_AggStep, nArg, regAgg, pF->iMem);
+		sqlVdbeAppendP4(v, ctx, P4_FUNCCTX);
 		sql_expr_type_cache_change(pParse, regAgg, nArg);
 		sqlReleaseTempRange(pParse, regAgg, nArg);
 		if (addrNext) {
@@ -6744,3 +6749,21 @@ sql_expr_extract_select(struct Parse *parser, struct Select *select)
 	parser->parsed_ast.expr =
 		sqlExprDup(parser->db, expr_list->a->pExpr, 0);
 }
+
+struct sql_context *
+sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc)
+{
+	uint32_t size = sizeof(struct sql_context);
+	if (argc > 1)
+		size += (argc - 1) * sizeof(struct Mem);
+	struct sql_context *ctx = sqlDbMallocRawNN(sql_get(), size);
+	if (ctx == NULL)
+		return NULL;
+	ctx->pOut = NULL;
+	ctx->func = func;
+	ctx->is_aborted = false;
+	ctx->skipFlag = 0;
+	ctx->pVdbe = vdbe;
+	ctx->iOp = 0;
+	return ctx;
+}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2e250dc29..c2701dbde 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4141,6 +4141,9 @@ char *sqlStrAccumFinish(StrAccum *);
 void sqlStrAccumReset(StrAccum *);
 void sqlSelectDestInit(SelectDest *, int, int, int);
 
+struct sql_context *
+sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc);
+
 /*
  * Create an expression to load @a column from datasource
  * @a src_idx in @a src_list.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index a0f1be454..2ff7ce8f4 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1182,16 +1182,6 @@ case OP_CollSeq: {
 	break;
 }
 
-/* Opcode: BuiltinFunction0 P1 P2 P3 P4 *
- * Synopsis: r[P3]=func(r[P2@P1])
- *
- * Invoke a user function (P4 is a pointer to a FuncDef object that
- * defines the function) with P1 arguments taken from register P2 and
- * successors.  The result of the function is stored in register P3.
- * Register P3 must not be one of the function inputs.
- *
- * See also: BuiltinFunction, AggStep, AggFinal
- */
 /* Opcode: BuiltinFunction P1 P2 P3 P4 *
  * Synopsis: r[P3]=func(r[P2@P1])
  *
@@ -1200,37 +1190,8 @@ case OP_CollSeq: {
  * from register P2 and successors.  The result of the function is stored
  * in register P3.  Register P3 must not be one of the function inputs.
  *
- * SQL functions are initially coded as OP_BuiltinFunction0 with
- * P4 pointing to a FuncDef object.  But on first evaluation,
- * the P4 operand is automatically converted into an sql_context
- * object and the operation changed to this OP_BuiltinFunction
- * opcode.  In this way, the initialization of the sql_context
- * object occurs only once, rather than once for each evaluation
- * of the function.
- *
- * See also: BuiltinFunction0, AggStep, AggFinal
+ * See also: AggStep, AggFinal
  */
-case OP_BuiltinFunction0: {
-	int n;
-	sql_context *pCtx;
-
-	assert(pOp->p4type == P4_FUNC);
-	n = pOp->p1;
-	assert(pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor));
-	assert(n==0 || (pOp->p2>0 && pOp->p2+n<=(p->nMem+1 - p->nCursor)+1));
-	assert(pOp->p3<pOp->p2 || pOp->p3>=pOp->p2+n);
-	pCtx = sqlDbMallocRawNN(db, sizeof(*pCtx) + (n-1)*sizeof(sql_value*));
-	if (pCtx==0) goto no_mem;
-	pCtx->pOut = 0;
-	pCtx->func = pOp->p4.func;
-	pCtx->iOp = (int)(pOp - aOp);
-	pCtx->pVdbe = p;
-	pOp->p4type = P4_FUNCCTX;
-	pOp->p4.pCtx = pCtx;
-	pOp->opcode = OP_BuiltinFunction;
-	/* Fall through into OP_BuiltinFunction */
-	FALLTHROUGH;
-}
 case OP_BuiltinFunction: {
 	int i;
 	int argc = pOp->p1;
@@ -1238,6 +1199,7 @@ case OP_BuiltinFunction: {
 
 	assert(pOp->p4type==P4_FUNCCTX);
 	pCtx = pOp->p4.pCtx;
+	pCtx->iOp = (int)(pOp - aOp);
 
 	/* If this function is inside of a trigger, the register array in aMem[]
 	 * might change from one evaluation to the next.  The next block of code
@@ -4178,17 +4140,6 @@ case OP_DecrJumpZero: {      /* jump, in1 */
 }
 
 
-/* Opcode: AggStep0 P1 P2 P3 P4 *
- * Synopsis: accum=r[P3] step(r[P2@P1])
- *
- * Execute the step function for an aggregate.  The
- * function has P1 arguments.   P4 is a pointer to the FuncDef
- * structure that specifies the function.  Register P3 is the
- * accumulator.
- *
- * The P1 arguments are taken from register P2 and its
- * successors.
- */
 /* Opcode: AggStep P1 P2 P3 P4 *
  * Synopsis: accum=r[P3] step(r[P2@P1])
  *
@@ -4199,34 +4150,7 @@ case OP_DecrJumpZero: {      /* jump, in1 */
  *
  * The P1 arguments are taken from register P2 and its
  * successors.
- *
- * This opcode is initially coded as OP_AggStep0.  On first evaluation,
- * the FuncDef stored in P4 is converted into an sql_context and
- * the opcode is changed.  In this way, the initialization of the
- * sql_context only happens once, instead of on each call to the
- * step function.
  */
-case OP_AggStep0: {
-	int n;
-	sql_context *pCtx;
-
-	assert(pOp->p4type == P4_FUNC);
-	n = pOp->p5;
-	assert(pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor));
-	assert(n==0 || (pOp->p2>0 && pOp->p2+n<=(p->nMem+1 - p->nCursor)+1));
-	assert(pOp->p3<pOp->p2 || pOp->p3>=pOp->p2+n);
-	pCtx = sqlDbMallocRawNN(db, sizeof(*pCtx) + (n-1)*sizeof(sql_value*));
-	if (pCtx==0) goto no_mem;
-	pCtx->pMem = 0;
-	pCtx->func = pOp->p4.func;
-	pCtx->iOp = (int)(pOp - aOp);
-	pCtx->pVdbe = p;
-	pOp->p4type = P4_FUNCCTX;
-	pOp->p4.pCtx = pCtx;
-	pOp->opcode = OP_AggStep;
-	/* Fall through into OP_AggStep */
-	FALLTHROUGH;
-}
 case OP_AggStep: {
 	int i;
 	int argc = pOp->p1;
@@ -4236,6 +4160,7 @@ case OP_AggStep: {
 
 	assert(pOp->p4type==P4_FUNCCTX);
 	pCtx = pOp->p4.pCtx;
+	pCtx->iOp = (int)(pOp - aOp);
 	pMem = &aMem[pOp->p3];
 
 	/* If this function is inside of a trigger, the register array in aMem[]
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 8148ed8b0..e2dcc72ab 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1070,7 +1070,7 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
 		}
 #if defined(SQL_DEBUG) || defined(VDBE_PROFILE)
 	case P4_FUNCCTX:{
-			struct func *func = pOp->p4.func;
+			struct func *func = pOp->p4.pCtx->func;
 			sqlXPrintf(&x, "%s(%d)", func->def->name,
 				   func->def->param_count);
 			break;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 03/13] sql: move collation to struct sql_context
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
  2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 01/13] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches
  2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:01 ` Mergen Imeev via Tarantool-patches
  2021-09-14 21:22   ` Vladislav Shpilevoy via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 04/13] sql: introduce mem_append() Mergen Imeev via Tarantool-patches
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:01 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch makes it easier to get a collation by a function.

Needed for #4145
---
 src/box/sql/date.c    | 43 -------------------------------------------
 src/box/sql/expr.c    |  6 +-----
 src/box/sql/func.c    | 26 ++++++--------------------
 src/box/sql/select.c  | 12 +++++-------
 src/box/sql/sqlInt.h  |  3 +--
 src/box/sql/vdbe.c    | 18 +++---------------
 src/box/sql/vdbeInt.h |  4 +---
 src/box/sql/vdbeapi.c | 27 ---------------------------
 8 files changed, 17 insertions(+), 122 deletions(-)

diff --git a/src/box/sql/date.c b/src/box/sql/date.c
index dbf460498..914a00dd2 100644
--- a/src/box/sql/date.c
+++ b/src/box/sql/date.c
@@ -1247,46 +1247,3 @@ ctimestampFunc(sql_context * context,
 	datetimeFunc(context, 0, 0);
 }
 #endif				/* !defined(SQL_OMIT_DATETIME_FUNCS) */
-
-#ifdef SQL_OMIT_DATETIME_FUNCS
-/*
- * If the library is compiled to omit the full-scale date and time
- * handling (to get a smaller binary), the following minimal version
- * of the functions current_time(), current_date() and current_timestamp()
- * are included instead. This is to support column declarations that
- * include "DEFAULT CURRENT_TIME" etc.
- *
- * This function uses the C-library functions time(), gmtime()
- * and strftime(). The format string to pass to strftime() is supplied
- * as the user-data for the function.
- */
-static void
-currentTimeFunc(sql_context * context, int argc, sql_value ** argv)
-{
-	time_t t;
-	char *zFormat = (char *)sql_user_data(context);
-	sql_int64 iT;
-	struct tm *pTm;
-	struct tm sNow;
-	char zBuf[20];
-
-	UNUSED_PARAMETER(argc);
-	UNUSED_PARAMETER(argv);
-
-	iT = sqlStmtCurrentTime(context);
-	if (iT <= 0)
-		return;
-	t = iT / 1000 - 10000 * (sql_int64) 21086676;
-#if HAVE_GMTIME_R
-	pTm = gmtime_r(&t, &sNow);
-#else
-	pTm = gmtime(&t);
-	if (pTm)
-		memcpy(&sNow, pTm, sizeof(sNow));
-#endif
-	if (pTm) {
-		strftime(zBuf, 20, zFormat, &sNow);
-		sql_result_text(context, zBuf, -1, SQL_TRANSIENT);
-	}
-}
-#endif
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 6446ef091..ab7d95f7e 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4102,13 +4102,9 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 				pParse->is_aborted = true;
 				return 0;
 			}
-			if (sql_func_flag_is_set(func, SQL_FUNC_NEEDCOLL)) {
-				sqlVdbeAddOp4(v, OP_CollSeq, 0, 0, 0,
-						  (char *)coll, P4_COLLSEQ);
-			}
 			if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
 				struct sql_context *ctx =
-					sql_context_new(v, func, nFarg);
+					sql_context_new(func, nFarg, coll);
 				if (ctx == NULL) {
 					pParse->is_aborted = true;
 					return -1;
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 9009f9e4f..b4461c6ee 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -100,20 +100,6 @@ sql_func_uuid(struct sql_context *ctx, int argc, struct Mem **argv)
 	mem_set_uuid(ctx->pOut, &uuid);
 }
 
-/*
- * Return the collating function associated with a function.
- */
-static struct coll *
-sqlGetFuncCollSeq(sql_context * context)
-{
-	VdbeOp *pOp;
-	assert(context->pVdbe != 0);
-	pOp = &context->pVdbe->aOp[context->iOp - 1];
-	assert(pOp->opcode == OP_CollSeq);
-	assert(pOp->p4type == P4_COLLSEQ || pOp->p4.pColl == NULL);
-	return pOp->p4.pColl;
-}
-
 /*
  * Indicate that the accumulator load should be skipped on this
  * iteration of the aggregate loop.
@@ -141,7 +127,7 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
 		context->is_aborted = true;
 		return;
 	}
-	pColl = sqlGetFuncCollSeq(context);
+	pColl = context->coll;
 	assert(mask == -1 || mask == 0);
 	iBest = 0;
 	if (mem_is_null(argv[0]))
@@ -402,7 +388,7 @@ position_func(struct sql_context *context, int argc, struct Mem **argv)
 					       n_haystack_bytes);
 			}
 			int beg_offset = 0;
-			struct coll *coll = sqlGetFuncCollSeq(context);
+			struct coll *coll = context->coll;
 			int c;
 			for (c = 0; c + n_needle_chars <= n_haystack_chars; c++) {
 				if (coll->cmp((const char *) haystack_str + beg_offset,
@@ -667,7 +653,7 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv)   \
 		return;                                                        \
 	}                                                                      \
 	UErrorCode status = U_ZERO_ERROR;                                      \
-	struct coll *coll = sqlGetFuncCollSeq(context);                    \
+	struct coll *coll = context->coll;                                     \
 	const char *locale = NULL;                                             \
 	if (coll != NULL && coll->type == COLL_TYPE_ICU) {                     \
 		locale = ucol_getLocaleByType(coll->collator,                  \
@@ -1029,7 +1015,7 @@ likeFunc(sql_context *context, int argc, sql_value **argv)
 	if (!zA || !zB)
 		return;
 	int res;
-	struct coll *coll = sqlGetFuncCollSeq(context);
+	struct coll *coll = context->coll;
 	assert(coll != NULL);
 	res = sql_utf8_pattern_compare(zB, zA, zB_end, zA_end, coll, escape);
 
@@ -1050,7 +1036,7 @@ likeFunc(sql_context *context, int argc, sql_value **argv)
 static void
 nullifFunc(sql_context * context, int NotUsed, sql_value ** argv)
 {
-	struct coll *pColl = sqlGetFuncCollSeq(context);
+	struct coll *pColl = context->coll;
 	UNUSED_PARAMETER(NotUsed);
 	if (mem_cmp_scalar(argv[0], argv[1], pColl) != 0)
 		sql_result_value(context, argv[0]);
@@ -1761,7 +1747,7 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
 		if (!mem_is_null(pBest))
 			sqlSkipAccumulatorLoad(context);
 	} else if (!mem_is_null(pBest)) {
-		struct coll *pColl = sqlGetFuncCollSeq(context);
+		struct coll *pColl = context->coll;
 		/*
 		 * This step function is used for both the min()
 		 * and max() aggregates, the only difference
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index e27dcb336..d738ba137 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -5621,8 +5621,8 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
 			pParse->is_aborted = true;
 			return;
 		}
+		struct coll *coll = NULL;
 		if (sql_func_flag_is_set(pF->func, SQL_FUNC_NEEDCOLL)) {
-			struct coll *coll = NULL;
 			struct ExprList_item *pItem;
 			int j;
 			assert(pList != 0);	/* pList!=0 if pF->pFunc has NEEDCOLL */
@@ -5636,10 +5636,9 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
 			}
 			if (regHit == 0 && pAggInfo->nAccumulator)
 				regHit = ++pParse->nMem;
-			sqlVdbeAddOp4(v, OP_CollSeq, regHit, 0, 0,
-					  (char *)coll, P4_COLLSEQ);
+			sqlVdbeAddOp1(v, OP_SkipLoad, regHit);
 		}
-		struct sql_context *ctx = sql_context_new(v, pF->func, nArg);
+		struct sql_context *ctx = sql_context_new(pF->func, nArg, coll);
 		if (ctx == NULL) {
 			pParse->is_aborted = true;
 			return;
@@ -6751,7 +6750,7 @@ sql_expr_extract_select(struct Parse *parser, struct Select *select)
 }
 
 struct sql_context *
-sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc)
+sql_context_new(struct func *func, uint32_t argc, struct coll *coll)
 {
 	uint32_t size = sizeof(struct sql_context);
 	if (argc > 1)
@@ -6763,7 +6762,6 @@ sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc)
 	ctx->func = func;
 	ctx->is_aborted = false;
 	ctx->skipFlag = 0;
-	ctx->pVdbe = vdbe;
-	ctx->iOp = 0;
+	ctx->coll = coll;
 	return ctx;
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index c2701dbde..710bd86cf 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4142,7 +4142,7 @@ void sqlStrAccumReset(StrAccum *);
 void sqlSelectDestInit(SelectDest *, int, int, int);
 
 struct sql_context *
-sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc);
+sql_context_new(struct func *func, uint32_t argc, struct coll *coll);
 
 /*
  * Create an expression to load @a column from datasource
@@ -4204,7 +4204,6 @@ void sqlParser(void *, int, Token, Parse *);
 int sqlParserStackPeak(void *);
 #endif
 
-sql_int64 sqlStmtCurrentTime(sql_context *);
 int sqlVdbeParameterIndex(Vdbe *, const char *, int);
 int sqlTransferBindings(sql_stmt *, sql_stmt *);
 int sqlReprepare(Vdbe *);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 2ff7ce8f4..12dc9126b 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1159,23 +1159,13 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 	break;
 }
 
-/* Opcode: CollSeq P1 * * P4
- *
- * P4 is a pointer to a CollSeq struct. If the next call to a user function
- * or aggregate calls sqlGetFuncCollSeq(), this collation sequence will
- * be returned. This is used by the built-in min(), max() and nullif()
- * functions.
+/* Opcode: SkipLoad P1 * * * *
  *
  * If P1 is not zero, then it is a register that a subsequent min() or
  * max() aggregate will set to true if the current row is not the minimum or
  * maximum.  The P1 register is initialized to false by this instruction.
- *
- * The interface used by the implementation of the aforementioned functions
- * to retrieve the collation sequence set by this opcode is not available
- * publicly.  Only built-in functions have access to this feature.
  */
-case OP_CollSeq: {
-	assert(pOp->p4type==P4_COLLSEQ || pOp->p4.pColl == NULL);
+case OP_SkipLoad: {
 	if (pOp->p1) {
 		mem_set_bool(&aMem[pOp->p1], false);
 	}
@@ -1199,7 +1189,6 @@ case OP_BuiltinFunction: {
 
 	assert(pOp->p4type==P4_FUNCCTX);
 	pCtx = pOp->p4.pCtx;
-	pCtx->iOp = (int)(pOp - aOp);
 
 	/* If this function is inside of a trigger, the register array in aMem[]
 	 * might change from one evaluation to the next.  The next block of code
@@ -4160,7 +4149,6 @@ case OP_AggStep: {
 
 	assert(pOp->p4type==P4_FUNCCTX);
 	pCtx = pOp->p4.pCtx;
-	pCtx->iOp = (int)(pOp - aOp);
 	pMem = &aMem[pOp->p3];
 
 	/* If this function is inside of a trigger, the register array in aMem[]
@@ -4195,7 +4183,7 @@ case OP_AggStep: {
 	}
 	assert(mem_is_null(&t));
 	if (pCtx->skipFlag) {
-		assert(pOp[-1].opcode==OP_CollSeq);
+		assert(pOp[-1].opcode == OP_SkipLoad);
 		i = pOp[-1].p1;
 		if (i) mem_set_bool(&aMem[i], true);
 	}
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 575ab3f3d..b9a18f1a1 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -173,9 +173,7 @@ struct sql_context {
 	/* A pointer to function implementation. */
 	struct func *func;
 	Mem *pMem;		/* Memory cell used to store aggregate context */
-	Vdbe *pVdbe;		/* The VM that owns this context */
-	/** Instruction number of OP_BuiltinFunction0. */
-	int iOp;
+	struct coll *coll;
 	/*
 	 * True, if an error occurred during the execution of the
 	 * function.
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 115940227..6e598513c 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -349,33 +349,6 @@ sql_context_db_handle(sql_context * p)
 	return p->pOut->db;
 }
 
-/*
- * Return the current time for a statement.  If the current time
- * is requested more than once within the same run of a single prepared
- * statement, the exact same time is returned for each invocation regardless
- * of the amount of time that elapses between invocations.  In other words,
- * the time returned is always the time of the first call.
- */
-sql_int64
-sqlStmtCurrentTime(sql_context * p)
-{
-	int rc;
-#ifndef SQL_ENABLE_OR_STAT4
-	sql_int64 *piTime = &p->pVdbe->iCurrentTime;
-	assert(p->pVdbe != 0);
-#else
-	sql_int64 iTime = 0;
-	sql_int64 *piTime =
-	    p->pVdbe != 0 ? &p->pVdbe->iCurrentTime : &iTime;
-#endif
-	if (*piTime == 0) {
-		rc = sqlOsCurrentTimeInt64(p->pOut->db->pVfs, piTime);
-		if (rc)
-			*piTime = 0;
-	}
-	return *piTime;
-}
-
 /*
  * Allocate or return the aggregate context for a user function.  A new
  * context is allocated on the first call.  Subsequent calls return the
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 04/13] sql: introduce mem_append()
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
                   ` (2 preceding siblings ...)
  2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 03/13] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:02 ` Mergen Imeev via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 05/13] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:02 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch introduces function mem_append(). This function appends the
given string to the end of the STRING or VARBINARY contained in MEM.
In case MEM needs to increase the size of allocated memory, additional
memory is allocated in an attempt to reduce the total number of
allocations. The main use of this function is to add multiple strings to
a single MEM.

Needed for #4145
---
 src/box/sql/mem.c     | 19 +++++++++++++++++++
 src/box/sql/mem.h     |  8 ++++++++
 src/box/sql/vdbeaux.c |  9 ++-------
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index b7af00723..ea116cb50 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -1953,6 +1953,25 @@ mem_move(struct Mem *to, struct Mem *from)
 	from->zMalloc = NULL;
 }
 
+int
+mem_append(struct Mem *mem, const char *value, uint32_t len)
+{
+	assert((mem->type & (MEM_TYPE_BIN | MEM_TYPE_STR)) != 0);
+	int new_size = mem->n + len;
+	if (((mem->flags & (MEM_Static | MEM_Dyn | MEM_Ephem)) != 0) ||
+	    mem->szMalloc < new_size) {
+		/*
+		 * Force exponential buffer size growth to avoid having to call
+		 * this routine too often.
+		 */
+		if (sqlVdbeMemGrow(mem, new_size + mem->n, 1) != 0)
+			return -1;
+	}
+	memcpy(&mem->z[mem->n], value, len);
+	mem->n = new_size;
+	return 0;
+}
+
 int
 mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 0da45b8af..54a1e931b 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -597,6 +597,14 @@ mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from);
 void
 mem_move(struct Mem *to, struct Mem *from);
 
+/**
+ * Append the given string to the end of the STRING or VARBINARY contained in
+ * MEM. In case MEM needs to increase the size of allocated memory, additional
+ * memory is allocated in an attempt to reduce the total number of allocations.
+ */
+int
+mem_append(struct Mem *mem, const char *value, uint32_t len);
+
 /**
  * Concatenate strings or binaries from the first and the second MEMs and write
  * to the result MEM. In case the first MEM or the second MEM is NULL, the
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index e2dcc72ab..482dcca0c 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1302,14 +1302,9 @@ sqlVdbeList(Vdbe * p)
 					if (mem_copy_bin(pSub, bin, size) != 0)
 						return -1;
 				} else if (j == nSub) {
-					struct Mem tmp;
-					mem_create(&tmp);
-					uint32_t size = sizeof(SubProgram *);
 					char *bin = (char *)&pOp->p4.pProgram;
-					mem_set_bin_ephemeral(&tmp, bin, size);
-					int rc = mem_concat(pSub, &tmp, pSub);
-					mem_destroy(&tmp);
-					if (rc != 0)
+					uint32_t size = sizeof(SubProgram *);
+					if (mem_append(pSub, bin, size) != 0)
 						return -1;
 				}
 			}
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 05/13] sql: remove sql_vdbemem_finalize()
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
                   ` (3 preceding siblings ...)
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 04/13] sql: introduce mem_append() Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:02 ` Mergen Imeev via Tarantool-patches
  2021-09-14 21:23   ` Vladislav Shpilevoy via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 06/13] sql: rework SUM() Mergen Imeev via Tarantool-patches
                   ` (7 subsequent siblings)
  12 siblings, 1 reply; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:02 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

We don't need this function, since it is easier to call finalizers
directly. This patch also allows us to make further simplifications.

Needed for #4145
---
 src/box/sql/mem.c  | 31 +------------------------------
 src/box/sql/mem.h  | 12 ------------
 src/box/sql/vdbe.c | 30 +++++++++++++++++++++++-------
 3 files changed, 24 insertions(+), 49 deletions(-)

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index ea116cb50..d3fce9cb7 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -217,11 +217,7 @@ mem_create(struct Mem *mem)
 static inline void
 mem_clear(struct Mem *mem)
 {
-	if ((mem->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0 ||
-	    (mem->flags & MEM_Dyn) != 0) {
-		if (mem->type == MEM_TYPE_AGG)
-			sql_vdbemem_finalize(mem, mem->u.func);
-		assert(mem->type != MEM_TYPE_AGG);
+	if ((mem->type & MEM_TYPE_FRAME) != 0 || (mem->flags & MEM_Dyn) != 0) {
 		if ((mem->flags & MEM_Dyn) != 0) {
 			assert(mem->xDel != SQL_DYNAMIC && mem->xDel != NULL);
 			mem->xDel((void *)mem->z);
@@ -3065,31 +3061,6 @@ sqlVdbeMemTooBig(Mem * p)
 	return 0;
 }
 
-int
-sql_vdbemem_finalize(struct Mem *mem, struct func *func)
-{
-	assert(func != NULL);
-	assert(func->def->language == FUNC_LANGUAGE_SQL_BUILTIN);
-	assert(func->def->aggregate == FUNC_AGGREGATE_GROUP);
-	assert(mem->type == MEM_TYPE_NULL || func == mem->u.func);
-	sql_context ctx;
-	memset(&ctx, 0, sizeof(ctx));
-	Mem t;
-	memset(&t, 0, sizeof(t));
-	t.type = MEM_TYPE_NULL;
-	assert(t.flags == 0);
-	t.db = mem->db;
-	ctx.pOut = &t;
-	ctx.pMem = mem;
-	ctx.func = func;
-	((struct func_sql_builtin *)func)->finalize(&ctx);
-	assert((mem->flags & MEM_Dyn) == 0);
-	if (mem->szMalloc > 0)
-		sqlDbFree(mem->db, mem->zMalloc);
-	memcpy(mem, &t, sizeof(t));
-	return ctx.is_aborted ? -1 : 0;
-}
-
 int
 sqlVdbeRecordCompareMsgpack(const void *key1,
 				struct UnpackedRecord *key2)
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 54a1e931b..c0993e573 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -969,18 +969,6 @@ int sqlVdbeMemTooBig(Mem *);
 #define VdbeMemDynamic(X) (((X)->flags & MEM_Dyn) != 0 ||\
 			   ((X)->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0)
 
-/** MEM manipulate functions. */
-
-/**
- * Memory cell mem contains the context of an aggregate function.
- * This routine calls the finalize method for that function. The
- * result of the aggregate is stored back into mem.
- *
- * Returns -1 if the finalizer reports an error. 0 otherwise.
- */
-int
-sql_vdbemem_finalize(struct Mem *mem, struct func *func);
-
 /**
  * Perform comparison of two tuples: unpacked (key1) and packed (key2)
  *
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 12dc9126b..dfab86c50 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4204,16 +4204,32 @@ case OP_AggStep: {
  * the step function was not previously called.
  */
 case OP_AggFinal: {
-	Mem *pMem;
 	assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
-	pMem = &aMem[pOp->p1];
-	assert(mem_is_null(pMem) || mem_is_agg(pMem));
-	if (sql_vdbemem_finalize(pMem, pOp->p4.func) != 0)
+	struct func_sql_builtin *func = (struct func_sql_builtin *)pOp->p4.func;
+	struct Mem *pIn1 = &aMem[pOp->p1];
+	assert(mem_is_null(pIn1) || mem_is_agg(pIn1));
+
+	struct sql_context ctx;
+	memset(&ctx, 0, sizeof(ctx));
+	struct Mem t;
+	memset(&t, 0, sizeof(t));
+	t.type = MEM_TYPE_NULL;
+	assert(t.flags == 0);
+	t.db = pIn1->db;
+	ctx.pOut = &t;
+	ctx.pMem = pIn1;
+	ctx.func = pOp->p4.func;
+	func->finalize(&ctx);
+	assert((pIn1->flags & MEM_Dyn) == 0);
+	if (pIn1->szMalloc > 0)
+		sqlDbFree(pIn1->db, pIn1->zMalloc);
+	memcpy(pIn1, &t, sizeof(t));
+
+	if (ctx.is_aborted)
 		goto abort_due_to_error;
-	UPDATE_MAX_BLOBSIZE(pMem);
-	if (sqlVdbeMemTooBig(pMem)) {
+	UPDATE_MAX_BLOBSIZE(pIn1);
+	if (sqlVdbeMemTooBig(pIn1) != 0)
 		goto too_big;
-	}
 	break;
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 06/13] sql: rework SUM()
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
                   ` (4 preceding siblings ...)
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 05/13] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:02 ` Mergen Imeev via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 07/13] sql: rework TOTAL() Mergen Imeev via Tarantool-patches
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:02 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch makes SUM() accept DOUBLE values by default. Also, after this
patch SUM() will be able to work with DECIMAL values.

Part of #4145
Part of #6355
---
 src/box/sql/func.c                       | 39 ++++++++++++++++--------
 src/box/sql/vdbe.c                       |  1 -
 test/sql-tap/built-in-functions.test.lua | 35 +++++++++++++++++++--
 3 files changed, 59 insertions(+), 16 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index b4461c6ee..b06955302 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -53,6 +53,29 @@
 static struct mh_strnptr_t *built_in_functions = NULL;
 static struct func_sql_builtin **functions;
 
+/** Implementation of the SUM() function. */
+static void
+step_sum(struct sql_context *ctx, int argc, struct Mem **argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_num(ctx->pMem));
+	if (argv[0]->type == MEM_TYPE_NULL)
+		return;
+	if (ctx->pMem->type == MEM_TYPE_NULL)
+		return mem_copy_as_ephemeral(ctx->pMem, argv[0]);
+	if (mem_add(ctx->pMem, argv[0], ctx->pMem) != 0)
+		ctx->is_aborted = true;
+}
+
+/** Finalizer for the SUM() function. */
+static void
+fin_sum(struct sql_context *ctx)
+{
+	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_num(ctx->pMem));
+	mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1654,17 +1677,6 @@ sum_step(struct sql_context *context, int argc, sql_value **argv)
 		context->is_aborted = true;
 }
 
-static void
-sumFinalize(sql_context * context)
-{
-	SumCtx *p;
-	p = sql_aggregate_context(context, 0);
-	if (p == NULL || p->count == 0)
-		mem_set_null(context->pOut);
-	else
-		mem_copy_as_ephemeral(context->pOut, &p->mem);
-}
-
 static void
 avgFinalize(sql_context * context)
 {
@@ -2113,8 +2125,9 @@ static struct sql_func_definition definitions[] = {
 	{"SUBSTR", 3,
 	 {FIELD_TYPE_VARBINARY, FIELD_TYPE_INTEGER, FIELD_TYPE_INTEGER},
 	 FIELD_TYPE_VARBINARY, substrFunc, NULL},
-	{"SUM", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, sum_step, sumFinalize},
-	{"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, sum_step, sumFinalize},
+	{"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_sum, fin_sum},
+	{"SUM", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_sum, fin_sum},
+	{"SUM", 1, {FIELD_TYPE_DECIMAL}, FIELD_TYPE_DECIMAL, step_sum, fin_sum},
 	{"TOTAL", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_DOUBLE, sum_step,
 	 totalFinalize},
 	{"TOTAL", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, sum_step,
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index dfab86c50..a74ee29ea 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4207,7 +4207,6 @@ case OP_AggFinal: {
 	assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
 	struct func_sql_builtin *func = (struct func_sql_builtin *)pOp->p4.func;
 	struct Mem *pIn1 = &aMem[pOp->p1];
-	assert(mem_is_null(pIn1) || mem_is_agg(pIn1));
 
 	struct sql_context ctx;
 	memset(&ctx, 0, sizeof(ctx));
diff --git a/test/sql-tap/built-in-functions.test.lua b/test/sql-tap/built-in-functions.test.lua
index 6fae811dc..8daf41c1d 100755
--- a/test/sql-tap/built-in-functions.test.lua
+++ b/test/sql-tap/built-in-functions.test.lua
@@ -1,6 +1,8 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(52)
+test:plan(55)
+
+local dec = require('decimal')
 
 --
 -- Make sure that number of arguments check is checked properly for SQL built-in
@@ -455,7 +457,7 @@ test:do_test(
         local res = {pcall(box.execute, [[SELECT SUM(?);]], {'1'})}
         return {tostring(res[3])}
     end, {
-        "Type mismatch: can not convert string('1') to integer"
+        "Type mismatch: can not convert string('1') to double"
     })
 
 test:do_catchsql_test(
@@ -545,4 +547,33 @@ test:do_test(
         {name = "COLUMN_2", type = "scalar"},
     })
 
+-- Make sure SUM() accepts and returns DOUBLE by default.
+test:do_test(
+    "builtins-4.1.1",
+    function()
+        return box.execute([[SELECT SUM(?);]], {1}).metadata
+    end, {
+        {name = "COLUMN_1", type = "double"},
+    })
+
+test:do_test(
+    "builtins-4.1.2",
+    function()
+        local res = {pcall(box.execute, [[SELECT SUM(?);]], {-1ULL})}
+        return {tostring(res[3])}
+    end, {
+        "Type mismatch: can not convert integer(18446744073709551615) to double"
+    })
+
+-- Make sure SUM() works with DECIMAL properly.
+test:do_execsql_test(
+    "builtins-4.1.3",
+    [[
+        SELECT SUM(cast(column_2 as DECIMAL)) from (values(1), (123.432));
+    ]],
+    {
+        dec.new(124.432)
+    }
+)
+
 test:finish_test()
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 07/13] sql: rework TOTAL()
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
                   ` (5 preceding siblings ...)
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 06/13] sql: rework SUM() Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:02 ` Mergen Imeev via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 08/13] sql: rework AVG() Mergen Imeev via Tarantool-patches
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:02 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch makes TOTAL() accept DOUBLE values by default. Also, after
this patch TOTAL() will be able to work with DECIMAL values.

Part of #4145
Part of #6355
---
 src/box/sql/func.c                       | 47 ++++++++++++++++--------
 test/sql-tap/built-in-functions.test.lua | 33 ++++++++++++++++-
 2 files changed, 63 insertions(+), 17 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index b06955302..12a6a5a2c 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -76,6 +76,32 @@ fin_sum(struct sql_context *ctx)
 	mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
 }
 
+/** Implementation of the TOTAL() function. */
+static void
+step_total(struct sql_context *ctx, int argc, struct Mem **argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_num(ctx->pMem));
+	if (argv[0]->type == MEM_TYPE_NULL)
+		return;
+	if (ctx->pMem->type == MEM_TYPE_NULL)
+		mem_set_double(ctx->pMem, 0.0);
+	if (mem_add(ctx->pMem, argv[0], ctx->pMem) != 0)
+		ctx->is_aborted = true;
+}
+
+/** Finalizer for the TOTAL() function. */
+static void
+fin_total(struct sql_context *ctx)
+{
+	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_double(ctx->pMem));
+	if (ctx->pMem->type == MEM_TYPE_NULL)
+		mem_set_double(ctx->pOut, 0.0);
+	else
+		mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1693,17 +1719,6 @@ avgFinalize(sql_context * context)
 		context->is_aborted = true;
 }
 
-static void
-totalFinalize(sql_context * context)
-{
-	SumCtx *p;
-	p = sql_aggregate_context(context, 0);
-	if (p == NULL || p->count == 0)
-		mem_set_double(context->pOut, 0.0);
-	else
-		mem_copy_as_ephemeral(context->pOut, &p->mem);
-}
-
 /*
  * The following structure keeps track of state information for the
  * count() aggregate function.
@@ -2128,10 +2143,12 @@ static struct sql_func_definition definitions[] = {
 	{"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_sum, fin_sum},
 	{"SUM", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_sum, fin_sum},
 	{"SUM", 1, {FIELD_TYPE_DECIMAL}, FIELD_TYPE_DECIMAL, step_sum, fin_sum},
-	{"TOTAL", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_DOUBLE, sum_step,
-	 totalFinalize},
-	{"TOTAL", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, sum_step,
-	 totalFinalize},
+	{"TOTAL", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_total,
+	 fin_total},
+	{"TOTAL", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_DOUBLE, step_total,
+	 fin_total},
+	{"TOTAL", 1, {FIELD_TYPE_DECIMAL}, FIELD_TYPE_DOUBLE, step_total,
+	 fin_total},
 
 	{"TRIM", 2, {FIELD_TYPE_STRING, FIELD_TYPE_INTEGER},
 	 FIELD_TYPE_STRING, trim_func, NULL},
diff --git a/test/sql-tap/built-in-functions.test.lua b/test/sql-tap/built-in-functions.test.lua
index 8daf41c1d..507d06549 100755
--- a/test/sql-tap/built-in-functions.test.lua
+++ b/test/sql-tap/built-in-functions.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(55)
+test:plan(58)
 
 local dec = require('decimal')
 
@@ -497,7 +497,7 @@ test:do_test(
         local res = {pcall(box.execute, [[SELECT TOTAL(?);]], {'1'})}
         return {tostring(res[3])}
     end, {
-        "Type mismatch: can not convert string('1') to integer"
+        "Type mismatch: can not convert string('1') to double"
     })
 
 --
@@ -576,4 +576,33 @@ test:do_execsql_test(
     }
 )
 
+-- Make sure TOTAL() accepts DOUBLE by default.
+test:do_test(
+    "builtins-4.2.1",
+    function()
+        return box.execute([[SELECT TOTAL(?);]], {1}).metadata
+    end, {
+        {name = "COLUMN_1", type = "double"},
+    })
+
+test:do_test(
+    "builtins-4.2.2",
+    function()
+        local res = {pcall(box.execute, [[SELECT TOTAL(?);]], {-1ULL})}
+        return {tostring(res[3])}
+    end, {
+        "Type mismatch: can not convert integer(18446744073709551615) to double"
+    })
+
+-- Make sure TOTAL() works with DECIMAL properly.
+test:do_execsql_test(
+    "builtins-4.2.3",
+    [[
+        SELECT TOTAL(cast(column_2 as DECIMAL)) from (values(1), (123.432));
+    ]],
+    {
+        124.432
+    }
+)
+
 test:finish_test()
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 08/13] sql: rework AVG()
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
                   ` (6 preceding siblings ...)
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 07/13] sql: rework TOTAL() Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:02 ` Mergen Imeev via Tarantool-patches
  2021-09-14 21:24   ` Vladislav Shpilevoy via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 09/13] sql: rework COUNT() Mergen Imeev via Tarantool-patches
                   ` (4 subsequent siblings)
  12 siblings, 1 reply; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:02 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch makes AVG() accept DOUBLE values by default. Also, after this
patch AVG() will be able to work with DECIMAL values.

Part of #4145
Part of #6355
---
 src/box/sql/func.c                       | 106 +++++++++--------------
 test/sql-tap/built-in-functions.test.lua |  33 ++++++-
 2 files changed, 72 insertions(+), 67 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 12a6a5a2c..9e0c09206 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -102,6 +102,44 @@ fin_total(struct sql_context *ctx)
 		mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
 }
 
+/** Implementation of the AVG() function. */
+static void
+step_avg(struct sql_context *ctx, int argc, struct Mem **argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_bin(ctx->pMem));
+	if (argv[0]->type == MEM_TYPE_NULL)
+		return;
+	if (ctx->pMem->type == MEM_TYPE_NULL) {
+		uint32_t size = 2 * sizeof(struct Mem);
+		struct Mem *mems = sqlDbMallocRawNN(sql_get(), size);
+		mem_create(&mems[0]);
+		mem_create(&mems[1]);
+		mem_copy_as_ephemeral(&mems[0], argv[0]);
+		mem_set_uint(&mems[1], 1);
+		mem_set_bin_allocated(ctx->pMem, (char *)mems, size);
+		return;
+	}
+	struct Mem *mems = (struct Mem *)ctx->pMem->z;
+	assert(mems[1].type = MEM_TYPE_UINT);
+	++mems[1].u.u;
+	if (mem_add(&mems[0], argv[0], &mems[0]) != 0)
+		ctx->is_aborted = true;
+}
+
+/** Finalizer for the AVG() function. */
+static void
+fin_avg(struct sql_context *ctx)
+{
+	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_bin(ctx->pMem));
+	if (ctx->pMem->type == MEM_TYPE_NULL)
+		return mem_set_null(ctx->pOut);
+	struct Mem *mems = (struct Mem *)ctx->pMem->z;
+	if (mem_div(&mems[0], &mems[1], ctx->pOut) != 0)
+		ctx->is_aborted = true;
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1656,69 +1694,6 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
 	}
 }
 
-/*
- * An instance of the following structure holds the context of a
- * sum() or avg() aggregate computation.
- */
-typedef struct SumCtx SumCtx;
-struct SumCtx {
-	struct Mem mem;
-	uint32_t count;
-};
-
-/*
- * Routines used to compute the sum, average, and total.
- *
- * The SUM() function follows the (broken) SQL standard which means
- * that it returns NULL if it sums over no inputs.  TOTAL returns
- * 0.0 in that case.  In addition, TOTAL always returns a float where
- * SUM might return an integer if it never encounters a floating point
- * value.  TOTAL never fails, but SUM might through an exception if
- * it overflows an integer.
- */
-static void
-sum_step(struct sql_context *context, int argc, sql_value **argv)
-{
-	assert(argc == 1);
-	UNUSED_PARAMETER(argc);
-	struct SumCtx *p = sql_aggregate_context(context, sizeof(*p));
-	if (p == NULL) {
-		context->is_aborted = true;
-		return;
-	}
-	if (p->count == 0) {
-		mem_create(&p->mem);
-		assert(context->func->def->returns == FIELD_TYPE_INTEGER ||
-		       context->func->def->returns == FIELD_TYPE_DOUBLE);
-		if (context->func->def->returns == FIELD_TYPE_INTEGER)
-			mem_set_uint(&p->mem, 0);
-		else
-			mem_set_double(&p->mem, 0.0);
-	}
-	if (argv[0]->type == MEM_TYPE_NULL)
-		return;
-	++p->count;
-	assert(mem_is_num(argv[0]));
-	if (mem_add(&p->mem, argv[0], &p->mem) != 0)
-		context->is_aborted = true;
-}
-
-static void
-avgFinalize(sql_context * context)
-{
-	SumCtx *p;
-	p = sql_aggregate_context(context, 0);
-	if (p == NULL || p->count == 0) {
-		mem_set_null(context->pOut);
-		return;
-	}
-	struct Mem mem;
-	mem_create(&mem);
-	mem_set_uint(&mem, p->count);
-	if (mem_div(&p->mem, &mem, context->pOut) != 0)
-		context->is_aborted = true;
-}
-
 /*
  * The following structure keeps track of state information for the
  * count() aggregate function.
@@ -2015,8 +1990,9 @@ struct sql_func_definition {
 static struct sql_func_definition definitions[] = {
 	{"ABS", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, absFunc, NULL},
 	{"ABS", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, absFunc, NULL},
-	{"AVG", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, sum_step, avgFinalize},
-	{"AVG", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, sum_step, avgFinalize},
+	{"AVG", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_avg, fin_avg},
+	{"AVG", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_avg, fin_avg},
+	{"AVG", 1, {FIELD_TYPE_DECIMAL}, FIELD_TYPE_DECIMAL, step_avg, fin_avg},
 	{"CHAR", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_STRING, charFunc, NULL},
 	{"CHAR_LENGTH", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, lengthFunc,
 	 NULL},
diff --git a/test/sql-tap/built-in-functions.test.lua b/test/sql-tap/built-in-functions.test.lua
index 507d06549..08a63b86d 100755
--- a/test/sql-tap/built-in-functions.test.lua
+++ b/test/sql-tap/built-in-functions.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(58)
+test:plan(61)
 
 local dec = require('decimal')
 
@@ -477,7 +477,7 @@ test:do_test(
         local res = {pcall(box.execute, [[SELECT AVG(?);]], {'1'})}
         return {tostring(res[3])}
     end, {
-        "Type mismatch: can not convert string('1') to integer"
+        "Type mismatch: can not convert string('1') to double"
     })
 
 test:do_catchsql_test(
@@ -605,4 +605,33 @@ test:do_execsql_test(
     }
 )
 
+-- Make sure AVG() accepts and returns DOUBLE by default.
+test:do_test(
+    "builtins-4.1.1",
+    function()
+        return box.execute([[SELECT AVG(?);]], {1}).metadata
+    end, {
+        {name = "COLUMN_1", type = "double"},
+    })
+
+test:do_test(
+    "builtins-4.1.2",
+    function()
+        local res = {pcall(box.execute, [[SELECT AVG(?);]], {-1ULL})}
+        return {tostring(res[3])}
+    end, {
+        "Type mismatch: can not convert integer(18446744073709551615) to double"
+    })
+
+-- Make sure AVG() works with DECIMAL properly.
+test:do_execsql_test(
+    "builtins-4.1.3",
+    [[
+        SELECT AVG(cast(column_2 as DECIMAL)) from (values(1), (123.432));
+    ]],
+    {
+        dec.new(62.216)
+    }
+)
+
 test:finish_test()
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 09/13] sql: rework COUNT()
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
                   ` (7 preceding siblings ...)
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 08/13] sql: rework AVG() Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:02 ` Mergen Imeev via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 10/13] sql: rework MIN() and MAX() Mergen Imeev via Tarantool-patches
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:02 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch does some refactoring for the SQL built-in aggregate function
COUNT().

Part of #4145
---
 src/box/sql/func.c | 64 +++++++++++++++++++---------------------------
 1 file changed, 26 insertions(+), 38 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 9e0c09206..0723a08ba 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -140,6 +140,29 @@ fin_avg(struct sql_context *ctx)
 		ctx->is_aborted = true;
 }
 
+/** Implementation of the COUNT() function. */
+static void
+step_count(struct sql_context *ctx, int argc, struct Mem **argv)
+{
+	assert(argc == 0 || argc == 1);
+	if (ctx->pMem->type == MEM_TYPE_NULL)
+		mem_set_uint(ctx->pMem, 0);
+	if (argc == 1 && argv[0]->type == MEM_TYPE_NULL)
+		return;
+	assert(ctx->pMem->type == MEM_TYPE_UINT);
+	++ctx->pMem->u.u;
+}
+
+/** Finalizer for the COUNT() function. */
+static void
+fin_count(struct sql_context *ctx)
+{
+	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_uint(ctx->pMem));
+	if (ctx->pMem->type == MEM_TYPE_NULL)
+		return mem_set_uint(ctx->pOut, 0);
+	mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1694,41 +1717,6 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
 	}
 }
 
-/*
- * The following structure keeps track of state information for the
- * count() aggregate function.
- */
-typedef struct CountCtx CountCtx;
-struct CountCtx {
-	i64 n;
-};
-
-/*
- * Routines to implement the count() aggregate function.
- */
-static void
-countStep(sql_context * context, int argc, sql_value ** argv)
-{
-	CountCtx *p;
-	if (argc != 0 && argc != 1) {
-		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT,
-			 "COUNT", "0 or 1", argc);
-		context->is_aborted = true;
-		return;
-	}
-	p = sql_aggregate_context(context, sizeof(*p));
-	if ((argc == 0 || !mem_is_null(argv[0])) && p != NULL)
-		p->n++;
-}
-
-static void
-countFinalize(sql_context * context)
-{
-	CountCtx *p;
-	p = sql_aggregate_context(context, 0);
-	sql_result_uint(context, p ? p->n : 0);
-}
-
 /*
  * Routines to implement min() and max() aggregate functions.
  */
@@ -1998,9 +1986,9 @@ static struct sql_func_definition definitions[] = {
 	 NULL},
 	{"COALESCE", -1, {FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR, sql_builtin_stub,
 	 NULL},
-	{"COUNT", 0, {}, FIELD_TYPE_INTEGER, countStep, countFinalize},
-	{"COUNT", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_INTEGER, countStep,
-	 countFinalize},
+	{"COUNT", 0, {}, FIELD_TYPE_INTEGER, step_count, fin_count},
+	{"COUNT", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_INTEGER, step_count,
+	 fin_count},
 
 	{"GREATEST", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, minmaxFunc,
 	 NULL},
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 10/13] sql: rework MIN() and MAX()
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
                   ` (8 preceding siblings ...)
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 09/13] sql: rework COUNT() Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:02 ` Mergen Imeev via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 11/13] sql: rework GROUP_CONCAT() Mergen Imeev via Tarantool-patches
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:02 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch does some refactoring for the SQL built-in aggregate
functions MIN() and MAX().

Part of #4145
---
 src/box/sql/func.c | 162 +++++++++++++++++++--------------------------
 src/box/sql/vdbe.c |   1 -
 2 files changed, 69 insertions(+), 94 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 0723a08ba..a814228bb 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -163,6 +163,46 @@ fin_count(struct sql_context *ctx)
 	mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
 }
 
+/** Implementation of the MIN() and MAX() functions. */
+static void
+step_minmax(struct sql_context *ctx, int argc, struct Mem **argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	if (argv[0]->type == MEM_TYPE_NULL) {
+		if (ctx->pMem->type != MEM_TYPE_NULL)
+			ctx->skipFlag = 1;
+		return;
+	}
+	if (ctx->pMem->type == MEM_TYPE_NULL) {
+		if (mem_copy(ctx->pMem, argv[0]) != 0)
+			ctx->is_aborted = true;
+		return;
+	}
+
+	uint32_t flags = ((struct func_sql_builtin *)ctx->func)->flags;
+	bool is_max = (flags & SQL_FUNC_MAX) != 0;
+	/*
+	 * This step function is used for both the min() and max() aggregates,
+	 * the only difference between the two being that the sense of the
+	 * comparison is inverted.
+	 */
+	int cmp = mem_cmp_scalar(ctx->pMem, argv[0], ctx->coll);
+	if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
+		if (mem_copy(ctx->pMem, argv[0]) != 0)
+			ctx->is_aborted = true;
+		return;
+	}
+	ctx->skipFlag = 1;
+}
+
+/** Finalizer for the MIN() and MAX() functions. */
+static void
+fin_minmax(struct sql_context *ctx)
+{
+	mem_copy(ctx->pOut, ctx->pMem);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -210,16 +250,6 @@ sql_func_uuid(struct sql_context *ctx, int argc, struct Mem **argv)
 	mem_set_uuid(ctx->pOut, &uuid);
 }
 
-/*
- * Indicate that the accumulator load should be skipped on this
- * iteration of the aggregate loop.
- */
-static void
-sqlSkipAccumulatorLoad(sql_context * context)
-{
-	context->skipFlag = 1;
-}
-
 /*
  * Implementation of the non-aggregate min() and max() functions
  */
@@ -1717,60 +1747,6 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
 	}
 }
 
-/*
- * Routines to implement min() and max() aggregate functions.
- */
-static void
-minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
-{
-	Mem *pArg = (Mem *) argv[0];
-	Mem *pBest;
-	UNUSED_PARAMETER(NotUsed);
-
-	struct func_sql_builtin *func =
-		(struct func_sql_builtin *)context->func;
-	pBest = sql_context_agg_mem(context);
-	if (!pBest)
-		return;
-
-	if (mem_is_null(argv[0])) {
-		if (!mem_is_null(pBest))
-			sqlSkipAccumulatorLoad(context);
-	} else if (!mem_is_null(pBest)) {
-		struct coll *pColl = context->coll;
-		/*
-		 * This step function is used for both the min()
-		 * and max() aggregates, the only difference
-		 * between the two being that the sense of the
-		 * comparison is inverted.
-		 */
-		bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
-		int cmp = mem_cmp_scalar(pBest, pArg, pColl);
-		if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
-			if (mem_copy(pBest, pArg) != 0)
-				context->is_aborted = true;
-		} else {
-			sqlSkipAccumulatorLoad(context);
-		}
-	} else {
-		pBest->db = sql_context_db_handle(context);
-		if (mem_copy(pBest, pArg) != 0)
-			context->is_aborted = true;
-	}
-}
-
-static void
-minMaxFinalize(sql_context * context)
-{
-	struct Mem *mem = context->pMem;
-	struct Mem *res;
-	if (mem_get_agg(mem, (void **)&res) != 0)
-		return;
-	if (!mem_is_null(res))
-		sql_result_value(context, res);
-	mem_destroy(res);
-}
-
 /*
  * group_concat(EXPR, ?SEPARATOR?)
  */
@@ -2042,35 +2018,35 @@ static struct sql_func_definition definitions[] = {
 	{"LOWER", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, LowerICUFunc,
 	 NULL},
 
-	{"MAX", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, minmaxStep,
-	 minMaxFinalize},
-	{"MAX", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, minmaxStep,
-	 minMaxFinalize},
-	{"MAX", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, minmaxStep,
-	 minMaxFinalize},
-	{"MAX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, minmaxStep,
-	 minMaxFinalize},
-	{"MAX", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, minmaxStep,
-	 minMaxFinalize},
-	{"MAX", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, minmaxStep,
-	 minMaxFinalize},
-	{"MAX", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, minmaxStep,
-	 minMaxFinalize},
-
-	{"MIN", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, minmaxStep,
-	 minMaxFinalize},
-	{"MIN", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, minmaxStep,
-	 minMaxFinalize},
-	{"MIN", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, minmaxStep,
-	 minMaxFinalize},
-	{"MIN", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, minmaxStep,
-	 minMaxFinalize},
-	{"MIN", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, minmaxStep,
-	 minMaxFinalize},
-	{"MIN", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, minmaxStep,
-	 minMaxFinalize},
-	{"MIN", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, minmaxStep,
-	 minMaxFinalize},
+	{"MAX", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax,
+	 fin_minmax},
+	{"MAX", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_minmax,
+	 fin_minmax},
+	{"MAX", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, step_minmax,
+	 fin_minmax},
+	{"MAX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, step_minmax,
+	 fin_minmax},
+	{"MAX", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, step_minmax,
+	 fin_minmax},
+	{"MAX", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, step_minmax,
+	 fin_minmax},
+	{"MAX", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, step_minmax,
+	 fin_minmax},
+
+	{"MIN", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax,
+	 fin_minmax},
+	{"MIN", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_minmax,
+	 fin_minmax},
+	{"MIN", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, step_minmax,
+	 fin_minmax},
+	{"MIN", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, step_minmax,
+	 fin_minmax},
+	{"MIN", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, step_minmax,
+	 fin_minmax},
+	{"MIN", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, step_minmax,
+	 fin_minmax},
+	{"MIN", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, step_minmax,
+	 fin_minmax},
 
 	{"NULLIF", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR,
 	 nullifFunc, NULL},
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index a74ee29ea..8804e3d18 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4169,7 +4169,6 @@ case OP_AggStep: {
 	}
 #endif
 
-	pMem->n++;
 	mem_create(&t);
 	pCtx->pOut = &t;
 	pCtx->is_aborted = false;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 11/13] sql: rework GROUP_CONCAT()
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
                   ` (9 preceding siblings ...)
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 10/13] sql: rework MIN() and MAX() Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:02 ` Mergen Imeev via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 12/13] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 13/13] sql: remove MEM_TYPE_AGG Mergen Imeev via Tarantool-patches
  12 siblings, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:02 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch simplifies SQL built-in aggregate function GROUP_CONCAT().

Part of #4145
---
 src/box/sql/func.c | 121 +++++++++++++++++++--------------------------
 1 file changed, 50 insertions(+), 71 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index a814228bb..72c1c7828 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -203,6 +203,52 @@ fin_minmax(struct sql_context *ctx)
 	mem_copy(ctx->pOut, ctx->pMem);
 }
 
+/** Implementation of the GROUP_CONCAT() function. */
+static void
+step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
+{
+	assert(argc == 1 || argc == 2);
+	(void)argc;
+	if (argv[0]->type == MEM_TYPE_NULL)
+		return;
+	assert(mem_is_str(argv[0]) || mem_is_bin(argv[0]));
+	if (ctx->pMem->type == MEM_TYPE_NULL) {
+		if (mem_copy_str(ctx->pMem, argv[0]->z, argv[0]->n) != 0)
+			ctx->is_aborted = true;
+		return;
+	}
+	const char *sep = NULL;
+	int sep_len = 0;
+	if (argc == 1) {
+		sep = ",";
+		sep_len = 1;
+	} else if (argv[1]->type == MEM_TYPE_NULL) {
+		sep = "";
+		sep_len = 0;
+	} else {
+		assert(mem_is_same_type(argv[0], argv[0]));
+		sep = argv[1]->z;
+		sep_len = argv[1]->n;
+	}
+	if (sep_len > 0) {
+		if (mem_append(ctx->pMem, sep, sep_len) != 0) {
+			ctx->is_aborted = true;
+			return;
+		}
+	}
+	if (mem_append(ctx->pMem, argv[0]->z, argv[0]->n) != 0) {
+		ctx->is_aborted = true;
+		return;
+	}
+}
+
+/** Finalizer for the GROUP_CONCAT() function. */
+static void
+fin_group_concat(struct sql_context *ctx)
+{
+	mem_copy(ctx->pOut, ctx->pMem);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1747,73 +1793,6 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
 	}
 }
 
-/*
- * group_concat(EXPR, ?SEPARATOR?)
- */
-static void
-groupConcatStep(sql_context * context, int argc, sql_value ** argv)
-{
-	const char *zVal;
-	StrAccum *pAccum;
-	const char *zSep;
-	int nVal, nSep;
-	if (argc != 1 && argc != 2) {
-		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT,
-			 "GROUP_CONCAT", "1 or 2", argc);
-		context->is_aborted = true;
-		return;
-	}
-	if (mem_is_null(argv[0]))
-		return;
-	pAccum =
-	    (StrAccum *) sql_aggregate_context(context, sizeof(*pAccum));
-
-	if (pAccum) {
-		sql *db = sql_context_db_handle(context);
-		int firstTerm = pAccum->mxAlloc == 0;
-		pAccum->mxAlloc = db->aLimit[SQL_LIMIT_LENGTH];
-		if (!firstTerm) {
-			if (argc == 2) {
-				zSep = mem_as_str0(argv[1]);
-				nSep = mem_len_unsafe(argv[1]);
-			} else {
-				zSep = ",";
-				nSep = 1;
-			}
-			if (zSep)
-				sqlStrAccumAppend(pAccum, zSep, nSep);
-		}
-		zVal = mem_as_str0(argv[0]);
-		nVal = mem_len_unsafe(argv[0]);
-		if (zVal)
-			sqlStrAccumAppend(pAccum, zVal, nVal);
-	}
-}
-
-static void
-groupConcatFinalize(sql_context * context)
-{
-	StrAccum *pAccum;
-	pAccum = sql_aggregate_context(context, 0);
-	if (pAccum) {
-		if (pAccum->accError == STRACCUM_TOOBIG) {
-			diag_set(ClientError, ER_SQL_EXECUTE, "string or binary"\
-				 "string is too big");
-			context->is_aborted = true;
-		} else if (pAccum->accError == STRACCUM_NOMEM) {
-			context->is_aborted = true;
-		} else {
-			char *str = sqlStrAccumFinish(pAccum);
-			int len = pAccum->nChar;
-			assert(len >= 0);
-			if (context->func->def->returns == FIELD_TYPE_STRING)
-				mem_set_str_dynamic(context->pOut, str, len);
-			else
-				mem_set_bin_dynamic(context->pOut, str, len);
-		}
-	}
-}
-
 int
 sql_is_like_func(struct Expr *expr)
 {
@@ -1981,13 +1960,13 @@ static struct sql_func_definition definitions[] = {
 	 NULL},
 
 	{"GROUP_CONCAT", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING,
-	 groupConcatStep, groupConcatFinalize},
+	 step_group_concat, fin_group_concat},
 	{"GROUP_CONCAT", 2, {FIELD_TYPE_STRING, FIELD_TYPE_STRING},
-	 FIELD_TYPE_STRING, groupConcatStep, groupConcatFinalize},
+	 FIELD_TYPE_STRING, step_group_concat, fin_group_concat},
 	{"GROUP_CONCAT", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY,
-	 groupConcatStep, groupConcatFinalize},
+	 step_group_concat, fin_group_concat},
 	{"GROUP_CONCAT", 2, {FIELD_TYPE_VARBINARY, FIELD_TYPE_VARBINARY},
-	 FIELD_TYPE_VARBINARY, groupConcatStep, groupConcatFinalize},
+	 FIELD_TYPE_VARBINARY, step_group_concat, fin_group_concat},
 
 	{"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, hexFunc, NULL},
 	{"IFNULL", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR,
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 12/13] sql: remove copying of result in finalizers
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
                   ` (10 preceding siblings ...)
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 11/13] sql: rework GROUP_CONCAT() Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:02 ` Mergen Imeev via Tarantool-patches
  2021-09-14 21:24   ` Vladislav Shpilevoy via Tarantool-patches
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 13/13] sql: remove MEM_TYPE_AGG Mergen Imeev via Tarantool-patches
  12 siblings, 1 reply; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:02 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch removes copying of the result in the finalizers of the SQL
built-in aggregate functions.

Part of #4145
---
 src/box/sql/func.c    | 149 ++++++++++++++++--------------------------
 src/box/sql/sqlInt.h  |  12 ----
 src/box/sql/vdbe.c    |  39 ++++-------
 src/box/sql/vdbeInt.h |   1 -
 src/box/sql/vdbeapi.c |  39 -----------
 5 files changed, 67 insertions(+), 173 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 72c1c7828..f1f08e7d6 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -59,35 +59,27 @@ step_sum(struct sql_context *ctx, int argc, struct Mem **argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_num(ctx->pMem));
+	assert(ctx->pOut->type == MEM_TYPE_NULL || mem_is_num(ctx->pOut));
 	if (argv[0]->type == MEM_TYPE_NULL)
 		return;
-	if (ctx->pMem->type == MEM_TYPE_NULL)
-		return mem_copy_as_ephemeral(ctx->pMem, argv[0]);
-	if (mem_add(ctx->pMem, argv[0], ctx->pMem) != 0)
+	if (ctx->pOut->type == MEM_TYPE_NULL)
+		return mem_copy_as_ephemeral(ctx->pOut, argv[0]);
+	if (mem_add(ctx->pOut, argv[0], ctx->pOut) != 0)
 		ctx->is_aborted = true;
 }
 
-/** Finalizer for the SUM() function. */
-static void
-fin_sum(struct sql_context *ctx)
-{
-	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_num(ctx->pMem));
-	mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
-}
-
 /** Implementation of the TOTAL() function. */
 static void
 step_total(struct sql_context *ctx, int argc, struct Mem **argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_num(ctx->pMem));
+	assert(ctx->pOut->type == MEM_TYPE_NULL || mem_is_num(ctx->pOut));
 	if (argv[0]->type == MEM_TYPE_NULL)
 		return;
-	if (ctx->pMem->type == MEM_TYPE_NULL)
-		mem_set_double(ctx->pMem, 0.0);
-	if (mem_add(ctx->pMem, argv[0], ctx->pMem) != 0)
+	if (ctx->pOut->type == MEM_TYPE_NULL)
+		mem_set_double(ctx->pOut, 0.0);
+	if (mem_add(ctx->pOut, argv[0], ctx->pOut) != 0)
 		ctx->is_aborted = true;
 }
 
@@ -95,11 +87,9 @@ step_total(struct sql_context *ctx, int argc, struct Mem **argv)
 static void
 fin_total(struct sql_context *ctx)
 {
-	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_double(ctx->pMem));
-	if (ctx->pMem->type == MEM_TYPE_NULL)
+	assert(ctx->pOut->type == MEM_TYPE_NULL || mem_is_double(ctx->pOut));
+	if (ctx->pOut->type == MEM_TYPE_NULL)
 		mem_set_double(ctx->pOut, 0.0);
-	else
-		mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
 }
 
 /** Implementation of the AVG() function. */
@@ -108,20 +98,20 @@ step_avg(struct sql_context *ctx, int argc, struct Mem **argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_bin(ctx->pMem));
+	assert(ctx->pOut->type == MEM_TYPE_NULL || mem_is_bin(ctx->pOut));
 	if (argv[0]->type == MEM_TYPE_NULL)
 		return;
-	if (ctx->pMem->type == MEM_TYPE_NULL) {
+	if (ctx->pOut->type == MEM_TYPE_NULL) {
 		uint32_t size = 2 * sizeof(struct Mem);
 		struct Mem *mems = sqlDbMallocRawNN(sql_get(), size);
 		mem_create(&mems[0]);
 		mem_create(&mems[1]);
 		mem_copy_as_ephemeral(&mems[0], argv[0]);
 		mem_set_uint(&mems[1], 1);
-		mem_set_bin_allocated(ctx->pMem, (char *)mems, size);
+		mem_set_bin_allocated(ctx->pOut, (char *)mems, size);
 		return;
 	}
-	struct Mem *mems = (struct Mem *)ctx->pMem->z;
+	struct Mem *mems = (struct Mem *)ctx->pOut->z;
 	assert(mems[1].type = MEM_TYPE_UINT);
 	++mems[1].u.u;
 	if (mem_add(&mems[0], argv[0], &mems[0]) != 0)
@@ -132,10 +122,10 @@ step_avg(struct sql_context *ctx, int argc, struct Mem **argv)
 static void
 fin_avg(struct sql_context *ctx)
 {
-	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_bin(ctx->pMem));
-	if (ctx->pMem->type == MEM_TYPE_NULL)
-		return mem_set_null(ctx->pOut);
-	struct Mem *mems = (struct Mem *)ctx->pMem->z;
+	assert(ctx->pOut->type == MEM_TYPE_NULL || mem_is_bin(ctx->pOut));
+	if (ctx->pOut->type == MEM_TYPE_NULL)
+		return;
+	struct Mem *mems = (struct Mem *)ctx->pOut->z;
 	if (mem_div(&mems[0], &mems[1], ctx->pOut) != 0)
 		ctx->is_aborted = true;
 }
@@ -145,22 +135,21 @@ static void
 step_count(struct sql_context *ctx, int argc, struct Mem **argv)
 {
 	assert(argc == 0 || argc == 1);
-	if (ctx->pMem->type == MEM_TYPE_NULL)
-		mem_set_uint(ctx->pMem, 0);
+	if (ctx->pOut->type == MEM_TYPE_NULL)
+		mem_set_uint(ctx->pOut, 0);
 	if (argc == 1 && argv[0]->type == MEM_TYPE_NULL)
 		return;
-	assert(ctx->pMem->type == MEM_TYPE_UINT);
-	++ctx->pMem->u.u;
+	assert(ctx->pOut->type == MEM_TYPE_UINT);
+	++ctx->pOut->u.u;
 }
 
 /** Finalizer for the COUNT() function. */
 static void
 fin_count(struct sql_context *ctx)
 {
-	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_uint(ctx->pMem));
-	if (ctx->pMem->type == MEM_TYPE_NULL)
+	assert(ctx->pOut->type == MEM_TYPE_NULL || mem_is_uint(ctx->pOut));
+	if (ctx->pOut->type == MEM_TYPE_NULL)
 		return mem_set_uint(ctx->pOut, 0);
-	mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
 }
 
 /** Implementation of the MIN() and MAX() functions. */
@@ -170,12 +159,12 @@ step_minmax(struct sql_context *ctx, int argc, struct Mem **argv)
 	assert(argc == 1);
 	(void)argc;
 	if (argv[0]->type == MEM_TYPE_NULL) {
-		if (ctx->pMem->type != MEM_TYPE_NULL)
+		if (ctx->pOut->type != MEM_TYPE_NULL)
 			ctx->skipFlag = 1;
 		return;
 	}
-	if (ctx->pMem->type == MEM_TYPE_NULL) {
-		if (mem_copy(ctx->pMem, argv[0]) != 0)
+	if (ctx->pOut->type == MEM_TYPE_NULL) {
+		if (mem_copy(ctx->pOut, argv[0]) != 0)
 			ctx->is_aborted = true;
 		return;
 	}
@@ -187,22 +176,15 @@ step_minmax(struct sql_context *ctx, int argc, struct Mem **argv)
 	 * the only difference between the two being that the sense of the
 	 * comparison is inverted.
 	 */
-	int cmp = mem_cmp_scalar(ctx->pMem, argv[0], ctx->coll);
+	int cmp = mem_cmp_scalar(ctx->pOut, argv[0], ctx->coll);
 	if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
-		if (mem_copy(ctx->pMem, argv[0]) != 0)
+		if (mem_copy(ctx->pOut, argv[0]) != 0)
 			ctx->is_aborted = true;
 		return;
 	}
 	ctx->skipFlag = 1;
 }
 
-/** Finalizer for the MIN() and MAX() functions. */
-static void
-fin_minmax(struct sql_context *ctx)
-{
-	mem_copy(ctx->pOut, ctx->pMem);
-}
-
 /** Implementation of the GROUP_CONCAT() function. */
 static void
 step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
@@ -212,8 +194,8 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
 	if (argv[0]->type == MEM_TYPE_NULL)
 		return;
 	assert(mem_is_str(argv[0]) || mem_is_bin(argv[0]));
-	if (ctx->pMem->type == MEM_TYPE_NULL) {
-		if (mem_copy_str(ctx->pMem, argv[0]->z, argv[0]->n) != 0)
+	if (ctx->pOut->type == MEM_TYPE_NULL) {
+		if (mem_copy_str(ctx->pOut, argv[0]->z, argv[0]->n) != 0)
 			ctx->is_aborted = true;
 		return;
 	}
@@ -231,24 +213,17 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
 		sep_len = argv[1]->n;
 	}
 	if (sep_len > 0) {
-		if (mem_append(ctx->pMem, sep, sep_len) != 0) {
+		if (mem_append(ctx->pOut, sep, sep_len) != 0) {
 			ctx->is_aborted = true;
 			return;
 		}
 	}
-	if (mem_append(ctx->pMem, argv[0]->z, argv[0]->n) != 0) {
+	if (mem_append(ctx->pOut, argv[0]->z, argv[0]->n) != 0) {
 		ctx->is_aborted = true;
 		return;
 	}
 }
 
-/** Finalizer for the GROUP_CONCAT() function. */
-static void
-fin_group_concat(struct sql_context *ctx)
-{
-	mem_copy(ctx->pOut, ctx->pMem);
-}
-
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1960,13 +1935,13 @@ static struct sql_func_definition definitions[] = {
 	 NULL},
 
 	{"GROUP_CONCAT", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING,
-	 step_group_concat, fin_group_concat},
+	 step_group_concat, NULL},
 	{"GROUP_CONCAT", 2, {FIELD_TYPE_STRING, FIELD_TYPE_STRING},
-	 FIELD_TYPE_STRING, step_group_concat, fin_group_concat},
+	 FIELD_TYPE_STRING, step_group_concat, NULL},
 	{"GROUP_CONCAT", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY,
-	 step_group_concat, fin_group_concat},
+	 step_group_concat, NULL},
 	{"GROUP_CONCAT", 2, {FIELD_TYPE_VARBINARY, FIELD_TYPE_VARBINARY},
-	 FIELD_TYPE_VARBINARY, step_group_concat, fin_group_concat},
+	 FIELD_TYPE_VARBINARY, step_group_concat, NULL},
 
 	{"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, hexFunc, NULL},
 	{"IFNULL", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR,
@@ -1997,35 +1972,23 @@ static struct sql_func_definition definitions[] = {
 	{"LOWER", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, LowerICUFunc,
 	 NULL},
 
-	{"MAX", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax,
-	 fin_minmax},
-	{"MAX", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_minmax,
-	 fin_minmax},
-	{"MAX", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, step_minmax,
-	 fin_minmax},
+	{"MAX", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax, NULL},
+	{"MAX", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_minmax, NULL},
+	{"MAX", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, step_minmax, NULL},
 	{"MAX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, step_minmax,
-	 fin_minmax},
-	{"MAX", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, step_minmax,
-	 fin_minmax},
-	{"MAX", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, step_minmax,
-	 fin_minmax},
-	{"MAX", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, step_minmax,
-	 fin_minmax},
-
-	{"MIN", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax,
-	 fin_minmax},
-	{"MIN", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_minmax,
-	 fin_minmax},
-	{"MIN", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, step_minmax,
-	 fin_minmax},
+	 NULL},
+	{"MAX", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, step_minmax, NULL},
+	{"MAX", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, step_minmax, NULL},
+	{"MAX", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, step_minmax, NULL},
+
+	{"MIN", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax, NULL},
+	{"MIN", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_minmax, NULL},
+	{"MIN", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, step_minmax, NULL},
 	{"MIN", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, step_minmax,
-	 fin_minmax},
-	{"MIN", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, step_minmax,
-	 fin_minmax},
-	{"MIN", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, step_minmax,
-	 fin_minmax},
-	{"MIN", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, step_minmax,
-	 fin_minmax},
+	 NULL},
+	{"MIN", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, step_minmax, NULL},
+	{"MIN", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, step_minmax, NULL},
+	{"MIN", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, step_minmax, NULL},
 
 	{"NULLIF", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR,
 	 nullifFunc, NULL},
@@ -2059,9 +2022,9 @@ static struct sql_func_definition definitions[] = {
 	{"SUBSTR", 3,
 	 {FIELD_TYPE_VARBINARY, FIELD_TYPE_INTEGER, FIELD_TYPE_INTEGER},
 	 FIELD_TYPE_VARBINARY, substrFunc, NULL},
-	{"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_sum, fin_sum},
-	{"SUM", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_sum, fin_sum},
-	{"SUM", 1, {FIELD_TYPE_DECIMAL}, FIELD_TYPE_DECIMAL, step_sum, fin_sum},
+	{"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_sum, NULL},
+	{"SUM", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_sum, NULL},
+	{"SUM", 1, {FIELD_TYPE_DECIMAL}, FIELD_TYPE_DECIMAL, step_sum, NULL},
 	{"TOTAL", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_total,
 	 fin_total},
 	{"TOTAL", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_DOUBLE, step_total,
@@ -2320,7 +2283,7 @@ sql_built_in_functions_cache_init(void)
 		assert(desc->argc != -1 || dict->argc_min != dict->argc_max);
 		def->param_count = desc->argc;
 		def->returns = desc->result;
-		def->aggregate = desc->finalize == NULL ?
+		def->aggregate = (dict->flags & SQL_FUNC_AGG) == 0 ?
 				 FUNC_AGGREGATE_NONE : FUNC_AGGREGATE_GROUP;
 		def->language = FUNC_LANGUAGE_SQL_BUILTIN;
 		def->name_len = len;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 710bd86cf..8782c072a 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -484,18 +484,6 @@ void
 sql_row_count(struct sql_context *context, MAYBE_UNUSED int unused1,
 	      MAYBE_UNUSED sql_value **unused2);
 
-void *
-sql_aggregate_context(sql_context *,
-			  int nBytes);
-
-/**
- * Allocate or return the aggregate context containing struct MEM for a user
- * function. A new context is allocated on the first call. Subsequent calls
- * return the same context that was returned on prior calls.
- */
-struct Mem *
-sql_context_agg_mem(struct sql_context *context);
-
 int
 sql_column_count(sql_stmt * pStmt);
 
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 8804e3d18..1d22c4a40 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4145,7 +4145,6 @@ case OP_AggStep: {
 	int argc = pOp->p1;
 	sql_context *pCtx;
 	Mem *pMem;
-	Mem t;
 
 	assert(pOp->p4type==P4_FUNCCTX);
 	pCtx = pOp->p4.pCtx;
@@ -4156,8 +4155,8 @@ case OP_AggStep: {
 	 * checks to see if the register array has changed, and if so it
 	 * reinitializes the relavant parts of the sql_context object
 	 */
-	if (pCtx->pMem != pMem) {
-		pCtx->pMem = pMem;
+	if (pCtx->pOut != pMem) {
+		pCtx->pOut = pMem;
 		for(i = 0; i < argc; ++i)
 			pCtx->argv[i] = &aMem[pOp->p2 + i];
 	}
@@ -4169,18 +4168,12 @@ case OP_AggStep: {
 	}
 #endif
 
-	mem_create(&t);
-	pCtx->pOut = &t;
-	pCtx->is_aborted = false;
 	pCtx->skipFlag = 0;
 	assert(pCtx->func->def->language == FUNC_LANGUAGE_SQL_BUILTIN);
 	struct func_sql_builtin *func = (struct func_sql_builtin *)pCtx->func;
 	func->call(pCtx, argc, pCtx->argv);
-	if (pCtx->is_aborted) {
-		mem_destroy(&t);
+	if (pCtx->is_aborted)
 		goto abort_due_to_error;
-	}
-	assert(mem_is_null(&t));
 	if (pCtx->skipFlag) {
 		assert(pOp[-1].opcode == OP_SkipLoad);
 		i = pOp[-1].p1;
@@ -4207,24 +4200,14 @@ case OP_AggFinal: {
 	struct func_sql_builtin *func = (struct func_sql_builtin *)pOp->p4.func;
 	struct Mem *pIn1 = &aMem[pOp->p1];
 
-	struct sql_context ctx;
-	memset(&ctx, 0, sizeof(ctx));
-	struct Mem t;
-	memset(&t, 0, sizeof(t));
-	t.type = MEM_TYPE_NULL;
-	assert(t.flags == 0);
-	t.db = pIn1->db;
-	ctx.pOut = &t;
-	ctx.pMem = pIn1;
-	ctx.func = pOp->p4.func;
-	func->finalize(&ctx);
-	assert((pIn1->flags & MEM_Dyn) == 0);
-	if (pIn1->szMalloc > 0)
-		sqlDbFree(pIn1->db, pIn1->zMalloc);
-	memcpy(pIn1, &t, sizeof(t));
-
-	if (ctx.is_aborted)
-		goto abort_due_to_error;
+	if (func->finalize != NULL) {
+		struct sql_context ctx;
+		memset(&ctx, 0, sizeof(ctx));
+		ctx.pOut = pIn1;
+		func->finalize(&ctx);
+		if (ctx.is_aborted)
+			goto abort_due_to_error;
+	}
 	UPDATE_MAX_BLOBSIZE(pIn1);
 	if (sqlVdbeMemTooBig(pIn1) != 0)
 		goto too_big;
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index b9a18f1a1..5e2692d06 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -172,7 +172,6 @@ struct sql_context {
 	Mem *pOut;		/* The return value is stored here */
 	/* A pointer to function implementation. */
 	struct func *func;
-	Mem *pMem;		/* Memory cell used to store aggregate context */
 	struct coll *coll;
 	/*
 	 * True, if an error occurred during the execution of the
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 6e598513c..39f560c32 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -349,45 +349,6 @@ sql_context_db_handle(sql_context * p)
 	return p->pOut->db;
 }
 
-/*
- * Allocate or return the aggregate context for a user function.  A new
- * context is allocated on the first call.  Subsequent calls return the
- * same context that was returned on prior calls.
- */
-void *
-sql_aggregate_context(sql_context * p, int nByte)
-{
-	assert(p != NULL && p->func != NULL);
-	assert(p->func->def->language == FUNC_LANGUAGE_SQL_BUILTIN);
-	assert(p->func->def->aggregate == FUNC_AGGREGATE_GROUP);
-	if (!mem_is_agg(p->pMem) && mem_set_agg(p->pMem, p->func, nByte) != 0)
-		return NULL;
-	void *accum;
-	if (mem_get_agg(p->pMem, &accum) != 0)
-		return NULL;
-	return accum;
-}
-
-struct Mem *
-sql_context_agg_mem(struct sql_context *ctx)
-{
-	assert(ctx != NULL && ctx->func != NULL);
-	assert(ctx->func->def->language == FUNC_LANGUAGE_SQL_BUILTIN);
-	assert(ctx->func->def->aggregate == FUNC_AGGREGATE_GROUP);
-	struct Mem *mem;
-	if (!mem_is_agg(ctx->pMem)) {
-		if (mem_set_agg(ctx->pMem, ctx->func, sizeof(*mem)) != 0)
-			return NULL;
-		if (mem_get_agg(ctx->pMem, (void **)&mem) != 0)
-			return NULL;
-		mem_create(mem);
-		return mem;
-	}
-	if (mem_get_agg(ctx->pMem, (void **)&mem) != 0)
-		return NULL;
-	return mem;
-}
-
 /*
  * Return the number of columns in the result set for the statement pStmt.
  */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Tarantool-patches] [PATCH v1 13/13] sql: remove MEM_TYPE_AGG
  2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
                   ` (11 preceding siblings ...)
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 12/13] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:02 ` Mergen Imeev via Tarantool-patches
  12 siblings, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:02 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch removed MEM_TYPE_AGG, which is no longer used due to recent
changes.

Part of #4145
---
 src/box/sql/mem.c | 25 -------------------------
 src/box/sql/mem.h |  9 +--------
 2 files changed, 1 insertion(+), 33 deletions(-)

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index d3fce9cb7..e4c6c22d9 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -605,22 +605,6 @@ mem_set_frame(struct Mem *mem, struct VdbeFrame *frame)
 	mem->u.pFrame = frame;
 }
 
-int
-mem_set_agg(struct Mem *mem, struct func *func, int size)
-{
-	mem_clear(mem);
-	if (size <= 0)
-		return 0;
-	if (sqlVdbeMemGrow(mem, size, 0) != 0)
-		return -1;
-	memset(mem->z, 0, size);
-	mem->n = size;
-	mem->type = MEM_TYPE_AGG;
-	assert(mem->flags == 0);
-	mem->u.func = func;
-	return 0;
-}
-
 void
 mem_set_null_clear(struct Mem *mem)
 {
@@ -1884,15 +1868,6 @@ mem_len(const struct Mem *mem, uint32_t *len)
 	return 0;
 }
 
-int
-mem_get_agg(const struct Mem *mem, void **accum)
-{
-	if (mem->type != MEM_TYPE_AGG)
-		return -1;
-	*accum = mem->z;
-	return 0;
-}
-
 int
 mem_copy(struct Mem *to, const struct Mem *from)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index c0993e573..9395f3224 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -54,7 +54,6 @@ enum mem_type {
 	MEM_TYPE_INVALID	= 1 << 11,
 	MEM_TYPE_FRAME		= 1 << 12,
 	MEM_TYPE_PTR		= 1 << 13,
-	MEM_TYPE_AGG		= 1 << 14,
 };
 
 /*
@@ -187,12 +186,6 @@ mem_is_array(const struct Mem *mem)
 	return mem->type == MEM_TYPE_ARRAY;
 }
 
-static inline bool
-mem_is_agg(const struct Mem *mem)
-{
-	return mem->type == MEM_TYPE_AGG;
-}
-
 static inline bool
 mem_is_bytes(const struct Mem *mem)
 {
@@ -967,7 +960,7 @@ int sqlVdbeMemTooBig(Mem *);
  * that needs to be deallocated to avoid a leak.
  */
 #define VdbeMemDynamic(X) (((X)->flags & MEM_Dyn) != 0 ||\
-			   ((X)->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0)
+			   ((X)->type & MEM_TYPE_FRAME) != 0)
 
 /**
  * Perform comparison of two tuples: unpacked (key1) and packed (key2)
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0
  2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
@ 2021-09-10 16:27   ` Mergen Imeev via Tarantool-patches
  2021-09-14 21:21   ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-10 16:27 UTC (permalink / raw)
  To: v.shpilevoy, tarantool-patches

Hi! Sorry, I found that there is one failing test in release built. I make some
changes, diff:


diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index e2dcc72ab..b7e44a386 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1068,14 +1068,12 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
 				   func->def->param_count);
 			break;
 		}
-#if defined(SQL_DEBUG) || defined(VDBE_PROFILE)
 	case P4_FUNCCTX:{
 			struct func *func = pOp->p4.pCtx->func;
 			sqlXPrintf(&x, "%s(%d)", func->def->name,
 				   func->def->param_count);
 			break;
 		}
-#endif
 	case P4_BOOL:
 			sqlXPrintf(&x, "%d", pOp->p4.b);
 			break;

On Fri, Sep 10, 2021 at 07:01:52PM +0300, Mergen Imeev via Tarantool-patches wrote:
> This patch moves the initialization of sql_context out of the VDBE. This
> allows us to remove the opcodes OP_BuiltinFunction0 and OP_AggStep0,
> which work in a rather strange way. Moreover, due to the changes these
> opcodes make to the VDBEs, it is possible that the estimated size of the
> VDBE may change during execution of VDBE, which could lead to various
> problems.
> 
> Needed for #4145
> ---
>  src/box/sql/expr.c    | 12 +++++--
>  src/box/sql/select.c  | 27 +++++++++++++--
>  src/box/sql/sqlInt.h  |  3 ++
>  src/box/sql/vdbe.c    | 81 ++-----------------------------------------
>  src/box/sql/vdbeaux.c |  2 +-
>  5 files changed, 41 insertions(+), 84 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 3b5df5292..6446ef091 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -4107,9 +4107,15 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>  						  (char *)coll, P4_COLLSEQ);
>  			}
>  			if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
> -				sqlVdbeAddOp4(v, OP_BuiltinFunction0, nFarg,
> -					      r1, target, (char *)func,
> -					      P4_FUNC);
> +				struct sql_context *ctx =
> +					sql_context_new(v, func, nFarg);
> +				if (ctx == NULL) {
> +					pParse->is_aborted = true;
> +					return -1;
> +				}
> +				sqlVdbeAddOp4(v, OP_BuiltinFunction, nFarg,
> +					      r1, target, (char *)ctx,
> +					      P4_FUNCCTX);
>  			} else {
>  				sqlVdbeAddOp4(v, OP_FunctionByName, nFarg,
>  					      r1, target,
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 92e40aef6..e27dcb336 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -5639,8 +5639,13 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
>  			sqlVdbeAddOp4(v, OP_CollSeq, regHit, 0, 0,
>  					  (char *)coll, P4_COLLSEQ);
>  		}
> -		sqlVdbeAddOp3(v, OP_AggStep0, nArg, regAgg, pF->iMem);
> -		sqlVdbeAppendP4(v, pF->func, P4_FUNC);
> +		struct sql_context *ctx = sql_context_new(v, pF->func, nArg);
> +		if (ctx == NULL) {
> +			pParse->is_aborted = true;
> +			return;
> +		}
> +		sqlVdbeAddOp3(v, OP_AggStep, nArg, regAgg, pF->iMem);
> +		sqlVdbeAppendP4(v, ctx, P4_FUNCCTX);
>  		sql_expr_type_cache_change(pParse, regAgg, nArg);
>  		sqlReleaseTempRange(pParse, regAgg, nArg);
>  		if (addrNext) {
> @@ -6744,3 +6749,21 @@ sql_expr_extract_select(struct Parse *parser, struct Select *select)
>  	parser->parsed_ast.expr =
>  		sqlExprDup(parser->db, expr_list->a->pExpr, 0);
>  }
> +
> +struct sql_context *
> +sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc)
> +{
> +	uint32_t size = sizeof(struct sql_context);
> +	if (argc > 1)
> +		size += (argc - 1) * sizeof(struct Mem);
> +	struct sql_context *ctx = sqlDbMallocRawNN(sql_get(), size);
> +	if (ctx == NULL)
> +		return NULL;
> +	ctx->pOut = NULL;
> +	ctx->func = func;
> +	ctx->is_aborted = false;
> +	ctx->skipFlag = 0;
> +	ctx->pVdbe = vdbe;
> +	ctx->iOp = 0;
> +	return ctx;
> +}
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 2e250dc29..c2701dbde 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4141,6 +4141,9 @@ char *sqlStrAccumFinish(StrAccum *);
>  void sqlStrAccumReset(StrAccum *);
>  void sqlSelectDestInit(SelectDest *, int, int, int);
>  
> +struct sql_context *
> +sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc);
> +
>  /*
>   * Create an expression to load @a column from datasource
>   * @a src_idx in @a src_list.
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index a0f1be454..2ff7ce8f4 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1182,16 +1182,6 @@ case OP_CollSeq: {
>  	break;
>  }
>  
> -/* Opcode: BuiltinFunction0 P1 P2 P3 P4 *
> - * Synopsis: r[P3]=func(r[P2@P1])
> - *
> - * Invoke a user function (P4 is a pointer to a FuncDef object that
> - * defines the function) with P1 arguments taken from register P2 and
> - * successors.  The result of the function is stored in register P3.
> - * Register P3 must not be one of the function inputs.
> - *
> - * See also: BuiltinFunction, AggStep, AggFinal
> - */
>  /* Opcode: BuiltinFunction P1 P2 P3 P4 *
>   * Synopsis: r[P3]=func(r[P2@P1])
>   *
> @@ -1200,37 +1190,8 @@ case OP_CollSeq: {
>   * from register P2 and successors.  The result of the function is stored
>   * in register P3.  Register P3 must not be one of the function inputs.
>   *
> - * SQL functions are initially coded as OP_BuiltinFunction0 with
> - * P4 pointing to a FuncDef object.  But on first evaluation,
> - * the P4 operand is automatically converted into an sql_context
> - * object and the operation changed to this OP_BuiltinFunction
> - * opcode.  In this way, the initialization of the sql_context
> - * object occurs only once, rather than once for each evaluation
> - * of the function.
> - *
> - * See also: BuiltinFunction0, AggStep, AggFinal
> + * See also: AggStep, AggFinal
>   */
> -case OP_BuiltinFunction0: {
> -	int n;
> -	sql_context *pCtx;
> -
> -	assert(pOp->p4type == P4_FUNC);
> -	n = pOp->p1;
> -	assert(pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor));
> -	assert(n==0 || (pOp->p2>0 && pOp->p2+n<=(p->nMem+1 - p->nCursor)+1));
> -	assert(pOp->p3<pOp->p2 || pOp->p3>=pOp->p2+n);
> -	pCtx = sqlDbMallocRawNN(db, sizeof(*pCtx) + (n-1)*sizeof(sql_value*));
> -	if (pCtx==0) goto no_mem;
> -	pCtx->pOut = 0;
> -	pCtx->func = pOp->p4.func;
> -	pCtx->iOp = (int)(pOp - aOp);
> -	pCtx->pVdbe = p;
> -	pOp->p4type = P4_FUNCCTX;
> -	pOp->p4.pCtx = pCtx;
> -	pOp->opcode = OP_BuiltinFunction;
> -	/* Fall through into OP_BuiltinFunction */
> -	FALLTHROUGH;
> -}
>  case OP_BuiltinFunction: {
>  	int i;
>  	int argc = pOp->p1;
> @@ -1238,6 +1199,7 @@ case OP_BuiltinFunction: {
>  
>  	assert(pOp->p4type==P4_FUNCCTX);
>  	pCtx = pOp->p4.pCtx;
> +	pCtx->iOp = (int)(pOp - aOp);
>  
>  	/* If this function is inside of a trigger, the register array in aMem[]
>  	 * might change from one evaluation to the next.  The next block of code
> @@ -4178,17 +4140,6 @@ case OP_DecrJumpZero: {      /* jump, in1 */
>  }
>  
>  
> -/* Opcode: AggStep0 P1 P2 P3 P4 *
> - * Synopsis: accum=r[P3] step(r[P2@P1])
> - *
> - * Execute the step function for an aggregate.  The
> - * function has P1 arguments.   P4 is a pointer to the FuncDef
> - * structure that specifies the function.  Register P3 is the
> - * accumulator.
> - *
> - * The P1 arguments are taken from register P2 and its
> - * successors.
> - */
>  /* Opcode: AggStep P1 P2 P3 P4 *
>   * Synopsis: accum=r[P3] step(r[P2@P1])
>   *
> @@ -4199,34 +4150,7 @@ case OP_DecrJumpZero: {      /* jump, in1 */
>   *
>   * The P1 arguments are taken from register P2 and its
>   * successors.
> - *
> - * This opcode is initially coded as OP_AggStep0.  On first evaluation,
> - * the FuncDef stored in P4 is converted into an sql_context and
> - * the opcode is changed.  In this way, the initialization of the
> - * sql_context only happens once, instead of on each call to the
> - * step function.
>   */
> -case OP_AggStep0: {
> -	int n;
> -	sql_context *pCtx;
> -
> -	assert(pOp->p4type == P4_FUNC);
> -	n = pOp->p5;
> -	assert(pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor));
> -	assert(n==0 || (pOp->p2>0 && pOp->p2+n<=(p->nMem+1 - p->nCursor)+1));
> -	assert(pOp->p3<pOp->p2 || pOp->p3>=pOp->p2+n);
> -	pCtx = sqlDbMallocRawNN(db, sizeof(*pCtx) + (n-1)*sizeof(sql_value*));
> -	if (pCtx==0) goto no_mem;
> -	pCtx->pMem = 0;
> -	pCtx->func = pOp->p4.func;
> -	pCtx->iOp = (int)(pOp - aOp);
> -	pCtx->pVdbe = p;
> -	pOp->p4type = P4_FUNCCTX;
> -	pOp->p4.pCtx = pCtx;
> -	pOp->opcode = OP_AggStep;
> -	/* Fall through into OP_AggStep */
> -	FALLTHROUGH;
> -}
>  case OP_AggStep: {
>  	int i;
>  	int argc = pOp->p1;
> @@ -4236,6 +4160,7 @@ case OP_AggStep: {
>  
>  	assert(pOp->p4type==P4_FUNCCTX);
>  	pCtx = pOp->p4.pCtx;
> +	pCtx->iOp = (int)(pOp - aOp);
>  	pMem = &aMem[pOp->p3];
>  
>  	/* If this function is inside of a trigger, the register array in aMem[]
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 8148ed8b0..e2dcc72ab 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -1070,7 +1070,7 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
>  		}
>  #if defined(SQL_DEBUG) || defined(VDBE_PROFILE)
>  	case P4_FUNCCTX:{
> -			struct func *func = pOp->p4.func;
> +			struct func *func = pOp->p4.pCtx->func;
>  			sqlXPrintf(&x, "%s(%d)", func->def->name,
>  				   func->def->param_count);
>  			break;
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0
  2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
  2021-09-10 16:27   ` Mergen Imeev via Tarantool-patches
@ 2021-09-14 21:21   ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 0 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-09-14 21:21 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hi! Thanks for the patch!

On 10.09.2021 18:01, imeevma@tarantool.org wrote:
> This patch moves the initialization of sql_context out of the VDBE. This
> allows us to remove the opcodes OP_BuiltinFunction0 and OP_AggStep0,
> which work in a rather strange way. Moreover, due to the changes these
> opcodes make to the VDBEs, it is possible that the estimated size of the
> VDBE may change during execution of VDBE, which could lead to various
> problems.

There should no be any problems if VDBE size is estimated right before it
is copied or whatever the size is used for. Because the updated VDBE would
return the updated size as well. Correct?

I must say, the old behaviour was quite inventive. Allowed to create an
object at runtime and yet not have ifs on the main opcodes paths. I think
the reason was to reduce usage of pointers in the opcodes. The goal we used
to have too.

I am fine with the change though.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 3b5df5292..6446ef091 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -6744,3 +6749,21 @@ sql_expr_extract_select(struct Parse *parser, struct Select *select)
>  	parser->parsed_ast.expr =
>  		sqlExprDup(parser->db, expr_list->a->pExpr, 0);
>  }
> +
> +struct sql_context *
> +sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc)
> +{
> +	uint32_t size = sizeof(struct sql_context);
> +	if (argc > 1)
> +		size += (argc - 1) * sizeof(struct Mem);

Why do you allocate + size of mem? I don't see where it is saved
here or used. ctx->pMem is left not initialized, pOut is zero. So
where is this size of mem used?

> +	struct sql_context *ctx = sqlDbMallocRawNN(sql_get(), size);
> +	if (ctx == NULL)
> +		return NULL;
> +	ctx->pOut = NULL;
> +	ctx->func = func;
> +	ctx->is_aborted = false;
> +	ctx->skipFlag = 0;
> +	ctx->pVdbe = vdbe;
> +	ctx->iOp = 0;
> +	return ctx;
> +}

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 03/13] sql: move collation to struct sql_context
  2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 03/13] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches
@ 2021-09-14 21:22   ` Vladislav Shpilevoy via Tarantool-patches
  2021-09-21 10:40     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-09-14 21:22 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Thanks for working on this!

On 10.09.2021 18:01, imeevma@tarantool.org wrote:
> This patch makes it easier to get a collation by a function.

You could also store the opcode pointer instead of iOp. But I
suspect your main reason is to get rid of the vdbe pointer? If
yes, then why?

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 2ff7ce8f4..12dc9126b 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1159,23 +1159,13 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
>  	break;
>  }
>  
> -/* Opcode: CollSeq P1 * * P4
> - *
> - * P4 is a pointer to a CollSeq struct. If the next call to a user function
> - * or aggregate calls sqlGetFuncCollSeq(), this collation sequence will
> - * be returned. This is used by the built-in min(), max() and nullif()
> - * functions.
> +/* Opcode: SkipLoad P1 * * * *
>   *
>   * If P1 is not zero, then it is a register that a subsequent min() or
>   * max() aggregate will set to true if the current row is not the minimum or
>   * maximum.  The P1 register is initialized to false by this instruction.
> - *
> - * The interface used by the implementation of the aforementioned functions
> - * to retrieve the collation sequence set by this opcode is not available
> - * publicly.  Only built-in functions have access to this feature.
>   */
> -case OP_CollSeq: {
> -	assert(pOp->p4type==P4_COLLSEQ || pOp->p4.pColl == NULL);
> +case OP_SkipLoad: {

That is a very strange name. Couldn't OP_Bool somehow be reused?
Why is this R[p1] = false even needed?

>  	if (pOp->p1) {
>  		mem_set_bool(&aMem[pOp->p1], false);
>  	}

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 05/13] sql: remove sql_vdbemem_finalize()
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 05/13] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches
@ 2021-09-14 21:23   ` Vladislav Shpilevoy via Tarantool-patches
  2021-09-21 10:47     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-09-14 21:23 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Good job on the patch!

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 12dc9126b..dfab86c50 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4204,16 +4204,32 @@ case OP_AggStep: {
>   * the step function was not previously called.
>   */
>  case OP_AggFinal: {
> -	Mem *pMem;
>  	assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
> -	pMem = &aMem[pOp->p1];
> -	assert(mem_is_null(pMem) || mem_is_agg(pMem));
> -	if (sql_vdbemem_finalize(pMem, pOp->p4.func) != 0)
> +	struct func_sql_builtin *func = (struct func_sql_builtin *)pOp->p4.func;
> +	struct Mem *pIn1 = &aMem[pOp->p1];
> +	assert(mem_is_null(pIn1) || mem_is_agg(pIn1));
> +
> +	struct sql_context ctx;
> +	memset(&ctx, 0, sizeof(ctx));
> +	struct Mem t;

Please, use mem_create(). Also for the context I would propose to add
sql_context_create(). So as not to touch internals of the struct here
directly.

> +	memset(&t, 0, sizeof(t));
> +	t.type = MEM_TYPE_NULL;
> +	assert(t.flags == 0);
> +	t.db = pIn1->db;
> +	ctx.pOut = &t;
> +	ctx.pMem = pIn1;
> +	ctx.func = pOp->p4.func;
> +	func->finalize(&ctx);
> +	assert((pIn1->flags & MEM_Dyn) == 0);
> +	if (pIn1->szMalloc > 0)
> +		sqlDbFree(pIn1->db, pIn1->zMalloc);
> +	memcpy(pIn1, &t, sizeof(t));
> +

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 08/13] sql: rework AVG()
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 08/13] sql: rework AVG() Mergen Imeev via Tarantool-patches
@ 2021-09-14 21:24   ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-09-14 21:24 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Thanks for the patch!

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 12a6a5a2c..9e0c09206 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -102,6 +102,44 @@ fin_total(struct sql_context *ctx)
>  		mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
>  }
>  
> +/** Implementation of the AVG() function. */
> +static void
> +step_avg(struct sql_context *ctx, int argc, struct Mem **argv)
> +{
> +	assert(argc == 1);
> +	(void)argc;
> +	assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_bin(ctx->pMem));
> +	if (argv[0]->type == MEM_TYPE_NULL)
> +		return;
> +	if (ctx->pMem->type == MEM_TYPE_NULL) {
> +		uint32_t size = 2 * sizeof(struct Mem);
> +		struct Mem *mems = sqlDbMallocRawNN(sql_get(), size);

Previously only size of mem + uint32 was allocated, now it is 2 size
of mem. Lets stick to the more compact version. For the division in the
end you can create a mem on the stack, for instance.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 12/13] sql: remove copying of result in finalizers
  2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 12/13] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches
@ 2021-09-14 21:24   ` Vladislav Shpilevoy via Tarantool-patches
  2021-09-21 10:49     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-09-14 21:24 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Thanks for the patch!

Shouldn't you also delete the sizeof(Mem) from the context size
in sql_context_new()?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 03/13] sql: move collation to struct sql_context
  2021-09-14 21:22   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-09-21 10:40     ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-21 10:40 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review! My answers below.

On Tue, Sep 14, 2021 at 11:22:39PM +0200, Vladislav Shpilevoy wrote:
> Thanks for working on this!
> 
> On 10.09.2021 18:01, imeevma@tarantool.org wrote:
> > This patch makes it easier to get a collation by a function.
> 
> You could also store the opcode pointer instead of iOp. But I
> suspect your main reason is to get rid of the vdbe pointer? If
> yes, then why?
> 
I wanted to remove unnecessary connecton between VDBE and functions. Also, this
way it will be easier for me to remove OP_CollSeq.

> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 2ff7ce8f4..12dc9126b 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -1159,23 +1159,13 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
> >  	break;
> >  }
> >  
> > -/* Opcode: CollSeq P1 * * P4
> > - *
> > - * P4 is a pointer to a CollSeq struct. If the next call to a user function
> > - * or aggregate calls sqlGetFuncCollSeq(), this collation sequence will
> > - * be returned. This is used by the built-in min(), max() and nullif()
> > - * functions.
> > +/* Opcode: SkipLoad P1 * * * *
> >   *
> >   * If P1 is not zero, then it is a register that a subsequent min() or
> >   * max() aggregate will set to true if the current row is not the minimum or
> >   * maximum.  The P1 register is initialized to false by this instruction.
> > - *
> > - * The interface used by the implementation of the aforementioned functions
> > - * to retrieve the collation sequence set by this opcode is not available
> > - * publicly.  Only built-in functions have access to this feature.
> >   */
> > -case OP_CollSeq: {
> > -	assert(pOp->p4type==P4_COLLSEQ || pOp->p4.pColl == NULL);
> > +case OP_SkipLoad: {
> 
> That is a very strange name. Couldn't OP_Bool somehow be reused?
> Why is this R[p1] = false even needed?
>
It is possible, for example like this:

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 2880f8ea0..c3f36d74f 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -5636,7 +5636,8 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
 			}
 			if (regHit == 0 && pAggInfo->nAccumulator)
 				regHit = ++pParse->nMem;
-			sqlVdbeAddOp1(v, OP_SkipLoad, regHit);
+			if (regHit != 0)
+				sqlVdbeAddOp2(v, OP_Bool, false, regHit);
 		}
 		struct sql_context *ctx = sql_context_new(pF->func, nArg, coll);
 		if (ctx == NULL) {
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 12dc9126b..b17179a57 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4182,9 +4182,8 @@ case OP_AggStep: {
 		goto abort_due_to_error;
 	}
 	assert(mem_is_null(&t));
-	if (pCtx->skipFlag) {
-		assert(pOp[-1].opcode == OP_SkipLoad);
-		i = pOp[-1].p1;
+	if (pCtx->skipFlag && pOp[-1].opcode == OP_Bool) {
+		i = pOp[-1].p2;
 		if (i) mem_set_bool(&aMem[i], true);
 	}
 	break;

However, I'm not sure if this is the correct way to use OP_Bool.

This opcode is used in rather strange queries, such as the following:
SELECT a, min(b) FROM t;

As you can see, there can be multiple values for min (b) in "a". This behavior is
discussed here:
https://github.com/tarantool/tarantool/discussions/6416

Most likely, we will follow the same path as PostgreSQL and MS SQL Server,
namely to prohibit such expressions. This will naturally lead to removal of
OP_SkipLoad/OP_CollSeq.

As for the name - I though of it when read desctiption of skipFlag field in
struct sql_context.

> >  	if (pOp->p1) {
> >  		mem_set_bool(&aMem[pOp->p1], false);
> >  	}


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 05/13] sql: remove sql_vdbemem_finalize()
  2021-09-14 21:23   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-09-21 10:47     ` Mergen Imeev via Tarantool-patches
  2021-09-22 22:47       ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-21 10:47 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Thank you for the review! My answer below.

On Tue, Sep 14, 2021 at 11:23:15PM +0200, Vladislav Shpilevoy wrote:
> Good job on the patch!
> 
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 12dc9126b..dfab86c50 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -4204,16 +4204,32 @@ case OP_AggStep: {
> >   * the step function was not previously called.
> >   */
> >  case OP_AggFinal: {
> > -	Mem *pMem;
> >  	assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
> > -	pMem = &aMem[pOp->p1];
> > -	assert(mem_is_null(pMem) || mem_is_agg(pMem));
> > -	if (sql_vdbemem_finalize(pMem, pOp->p4.func) != 0)
> > +	struct func_sql_builtin *func = (struct func_sql_builtin *)pOp->p4.func;
> > +	struct Mem *pIn1 = &aMem[pOp->p1];
> > +	assert(mem_is_null(pIn1) || mem_is_agg(pIn1));
> > +
> > +	struct sql_context ctx;
> > +	memset(&ctx, 0, sizeof(ctx));
> > +	struct Mem t;
> 
> Please, use mem_create(). Also for the context I would propose to add
> sql_context_create(). So as not to touch internals of the struct here
> directly.
> 
I suggest to leave it as it is for now, since I plan to completely drop
struct sql_context from finalize functions in a few patches in this patch-set.
What do you think?

> > +	memset(&t, 0, sizeof(t));
> > +	t.type = MEM_TYPE_NULL;
> > +	assert(t.flags == 0);
> > +	t.db = pIn1->db;
> > +	ctx.pOut = &t;
> > +	ctx.pMem = pIn1;
> > +	ctx.func = pOp->p4.func;
> > +	func->finalize(&ctx);
> > +	assert((pIn1->flags & MEM_Dyn) == 0);
> > +	if (pIn1->szMalloc > 0)
> > +		sqlDbFree(pIn1->db, pIn1->zMalloc);
> > +	memcpy(pIn1, &t, sizeof(t));
> > +

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 12/13] sql: remove copying of result in finalizers
  2021-09-14 21:24   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-09-21 10:49     ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 24+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-21 10:49 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Thank you for the review!

On Tue, Sep 14, 2021 at 11:24:18PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> Shouldn't you also delete the sizeof(Mem) from the context size
> in sql_context_new()?
No, but I will change from sizeof(struct Mem) to sizeof(struct Mem *). Also,
I will completely drop argv from struct sql_context in the new patch.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 05/13] sql: remove sql_vdbemem_finalize()
  2021-09-21 10:47     ` Mergen Imeev via Tarantool-patches
@ 2021-09-22 22:47       ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-09-22 22:47 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Thanks for the fixes!

>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>>> index 12dc9126b..dfab86c50 100644
>>> --- a/src/box/sql/vdbe.c
>>> +++ b/src/box/sql/vdbe.c
>>> @@ -4204,16 +4204,32 @@ case OP_AggStep: {
>>>   * the step function was not previously called.
>>>   */
>>>  case OP_AggFinal: {
>>> -	Mem *pMem;
>>>  	assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
>>> -	pMem = &aMem[pOp->p1];
>>> -	assert(mem_is_null(pMem) || mem_is_agg(pMem));
>>> -	if (sql_vdbemem_finalize(pMem, pOp->p4.func) != 0)
>>> +	struct func_sql_builtin *func = (struct func_sql_builtin *)pOp->p4.func;
>>> +	struct Mem *pIn1 = &aMem[pOp->p1];
>>> +	assert(mem_is_null(pIn1) || mem_is_agg(pIn1));
>>> +
>>> +	struct sql_context ctx;
>>> +	memset(&ctx, 0, sizeof(ctx));
>>> +	struct Mem t;
>>
>> Please, use mem_create(). Also for the context I would propose to add
>> sql_context_create(). So as not to touch internals of the struct here
>> directly.
>>
> I suggest to leave it as it is for now, since I plan to completely drop
> struct sql_context from finalize functions in a few patches in this patch-set.
> What do you think?

mem_create() would be just -3 and +1 line in the patch. I suggest to
use it. Previously memset was tolerated because it was done in mem.c,
where it is allowed to touch Mem internals directly.

>>> +	memset(&t, 0, sizeof(t));
>>> +	t.type = MEM_TYPE_NULL;
>>> +	assert(t.flags == 0);
>>> +	t.db = pIn1->db;
>>> +	ctx.pOut = &t;
>>> +	ctx.pMem = pIn1;
>>> +	ctx.func = pOp->p4.func;
>>> +	func->finalize(&ctx);
>>> +	assert((pIn1->flags & MEM_Dyn) == 0);
>>> +	if (pIn1->szMalloc > 0)
>>> +		sqlDbFree(pIn1->db, pIn1->zMalloc);
>>> +	memcpy(pIn1, &t, sizeof(t));
>>> +

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2021-09-22 22:47 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 01/13] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches
2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
2021-09-10 16:27   ` Mergen Imeev via Tarantool-patches
2021-09-14 21:21   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 03/13] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches
2021-09-14 21:22   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-21 10:40     ` Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 04/13] sql: introduce mem_append() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 05/13] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches
2021-09-14 21:23   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-21 10:47     ` Mergen Imeev via Tarantool-patches
2021-09-22 22:47       ` Vladislav Shpilevoy via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 06/13] sql: rework SUM() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 07/13] sql: rework TOTAL() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 08/13] sql: rework AVG() Mergen Imeev via Tarantool-patches
2021-09-14 21:24   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 09/13] sql: rework COUNT() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 10/13] sql: rework MIN() and MAX() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 11/13] sql: rework GROUP_CONCAT() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 12/13] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches
2021-09-14 21:24   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-21 10:49     ` Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 13/13] sql: remove MEM_TYPE_AGG Mergen Imeev via Tarantool-patches

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