Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions
@ 2021-10-01  7:42 Mergen Imeev via Tarantool-patches
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 01/15] sql: fix possible undefined behavior during cast Mergen Imeev via Tarantool-patches
                   ` (14 more replies)
  0 siblings, 15 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:42 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch-set refactor the aggregate functions and does some refactoring of the
SQL function infrastructure as a whole.

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

Changes in v2:
 - Added commit to remove possible UB in int-to-double implicit cast function.
 - Added commit to drop argv from struct sql_context.
 - Review fixes.

Changes in v3:
 - All functional changes were carried over from this patch-set. All that's left
   is refactoring.

Mergen Imeev (15):
  sql: fix possible undefined behavior during cast
  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: refactor SUM() function
  sql: refactor TOTAL() function
  sql: refactor AVG() function
  sql: refactor COUNT() function
  sql: refactor MIN() and MAX() functions
  sql: refactor GROUP_CONCAT() function
  sql: remove copying of result in finalizers
  sql: remove MEM_TYPE_AGG
  sql: remove field argv from struct sql_context

 src/box/sql/date.c         |  43 --
 src/box/sql/expr.c         |  19 +-
 src/box/sql/func.c         | 815 +++++++++++++++++--------------------
 src/box/sql/main.c         |   5 +-
 src/box/sql/mem.c          |  81 ++--
 src/box/sql/mem.h          |  29 +-
 src/box/sql/select.c       |  35 +-
 src/box/sql/sqlInt.h       |  26 +-
 src/box/sql/vdbe.c         | 192 ++-------
 src/box/sql/vdbeInt.h      |   7 +-
 src/box/sql/vdbeapi.c      |  66 ---
 src/box/sql/vdbeaux.c      |  21 +-
 test/sql-tap/func.test.lua |  15 +-
 13 files changed, 506 insertions(+), 848 deletions(-)

-- 
2.25.1


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

* [Tarantool-patches] [PATCH v3 01/15] sql: fix possible undefined behavior during cast
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:42 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 02/15] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches
                   ` (13 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:42 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch fixes possible undefined behavior during the implicit cast of
INTEGER to DOUBLE. The problem is, if the INTEGER is close enough to
2^64, it will be cast to 2^64 when it is cast to DOUBLE. Since we have a
check for loss of precision, this will cause this DOUBLE to be cast to
an INTEGER, which will result in undefined behavior since this DOUBLE is
outside the range of INTEGER.
---
 src/box/sql/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 5e23c901c..cc0fd836b 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -695,7 +695,7 @@ uint_to_double_precise(struct Mem *mem)
 	assert(mem->type == MEM_TYPE_UINT);
 	double d;
 	d = (double)mem->u.u;
-	if (mem->u.u != (uint64_t)d)
+	if (d == (double)UINT64_MAX || mem->u.u != (uint64_t)d)
 		return -1;
 	mem->u.r = d;
 	mem->flags = 0;
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v3 02/15] sql: use register P1 for number of arguments
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 01/15] sql: fix possible undefined behavior during cast Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:42 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 03/15] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
                   ` (12 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:42 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] 16+ messages in thread

* [Tarantool-patches] [PATCH v3 03/15] sql: remove AggStep0 and OP_BuiltinFunction0
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 01/15] sql: fix possible undefined behavior during cast Mergen Imeev via Tarantool-patches
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 02/15] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:42 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 04/15] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches
                   ` (11 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:42 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  | 33 ++++++++++++++++--
 src/box/sql/sqlInt.h  |  6 ++++
 src/box/sql/vdbe.c    | 81 ++-----------------------------------------
 src/box/sql/vdbeaux.c | 12 ++-----
 5 files changed, 51 insertions(+), 93 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..5b57f57a3 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,27 @@ 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;
+}
+
+void
+sql_context_delete(struct sql_context *ctx)
+{
+	sqlDbFree(sql_get(), ctx);
+}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2e250dc29..be31f2b50 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4141,6 +4141,12 @@ 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);
+
+void
+sql_context_delete(struct sql_context *ctx);
+
 /*
  * 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..4c2bd11ba 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -590,19 +590,13 @@ sqlVdbeJumpHere(Vdbe * p, int addr)
 
 static void vdbeFreeOpArray(sql *, Op *, int);
 
-static SQL_NOINLINE void
-freeP4FuncCtx(sql * db, sql_context * p)
-{
-	sqlDbFree(db, p);
-}
-
 static void
 freeP4(sql * db, int p4type, void *p4)
 {
 	assert(db);
 	switch (p4type) {
 	case P4_FUNCCTX:{
-			freeP4FuncCtx(db, (sql_context *) p4);
+			sql_context_delete(p4);
 			break;
 		}
 	case P4_REAL:
@@ -1068,14 +1062,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.func;
+			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;
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v3 04/15] sql: move collation to struct sql_context
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (2 preceding siblings ...)
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 03/15] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:42 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 05/15] sql: introduce mem_append() Mergen Imeev via Tarantool-patches
                   ` (10 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:42 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 29d713fd0..bfa0ad75b 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 5b57f57a3..6269c2868 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,8 +6762,7 @@ 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 be31f2b50..abcc3bfdf 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);
 
 void
 sql_context_delete(struct sql_context *ctx);
@@ -4207,7 +4207,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] 16+ messages in thread

* [Tarantool-patches] [PATCH v3 05/15] sql: introduce mem_append()
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (3 preceding siblings ...)
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 04/15] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:42 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 06/15] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches
                   ` (9 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:42 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch introduces the mem_append() function. This function appends
the specified string to the end of the STRING or VARBINARY contained in
MEM. In case MEM needs to increase the size of allocated memory,
extra memory is allocated in an attempt to reduce the total number of
allocations.

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

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index cc0fd836b..f64cbe10a 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -1953,6 +1953,27 @@ 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);
+	if (len == 0)
+		return 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 4c2bd11ba..3015760e1 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1294,14 +1294,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] 16+ messages in thread

* [Tarantool-patches] [PATCH v3 06/15] sql: remove sql_vdbemem_finalize()
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (4 preceding siblings ...)
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 05/15] sql: introduce mem_append() Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:42 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 07/15] sql: refactor SUM() function Mergen Imeev via Tarantool-patches
                   ` (8 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:42 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  | 33 ++-------------------------------
 src/box/sql/mem.h  | 12 ------------
 src/box/sql/vdbe.c | 27 ++++++++++++++++++++-------
 3 files changed, 22 insertions(+), 50 deletions(-)

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index f64cbe10a..e59935672 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -217,15 +217,11 @@ 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 || (mem->flags & MEM_Dyn) != 0) {
 		if ((mem->flags & MEM_Dyn) != 0) {
 			assert(mem->xDel != SQL_DYNAMIC && mem->xDel != NULL);
 			mem->xDel((void *)mem->z);
-		} else if (mem->type == MEM_TYPE_FRAME) {
+		} else {
 			struct VdbeFrame *frame = mem->u.pFrame;
 			frame->pParent = frame->v->pDelFrame;
 			frame->v->pDelFrame = frame;
@@ -3067,31 +3063,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..b101fc6c2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4204,16 +4204,29 @@ 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;
+	mem_create(&t);
+	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] 16+ messages in thread

* [Tarantool-patches] [PATCH v3 07/15] sql: refactor SUM() function
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (5 preceding siblings ...)
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 06/15] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:42 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 08/15] sql: refactor TOTAL() function Mergen Imeev via Tarantool-patches
                   ` (7 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:42 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

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

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index bfa0ad75b..9d96ac12b 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(mem_is_null(ctx->pMem) || mem_is_num(ctx->pMem));
+	if (mem_is_null(argv[0]))
+		return;
+	if (mem_is_null(ctx->pMem))
+		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(mem_is_null(ctx->pMem) || 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,8 @@ 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_INTEGER}, FIELD_TYPE_INTEGER, step_sum, fin_sum},
+	{"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, 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 b101fc6c2..66ac2c4f3 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));
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v3 08/15] sql: refactor TOTAL() function
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (6 preceding siblings ...)
  2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 07/15] sql: refactor SUM() function Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:43 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 09/15] sql: refactor AVG() function Mergen Imeev via Tarantool-patches
                   ` (6 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:43 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

Part of #4145
---
 src/box/sql/func.c | 45 ++++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 9d96ac12b..e809ea90c 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(mem_is_null(ctx->pMem) || mem_is_num(ctx->pMem));
+	if (mem_is_null(argv[0]))
+		return;
+	if (mem_is_null(ctx->pMem))
+		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(mem_is_null(ctx->pMem) || mem_is_double(ctx->pMem));
+	if (mem_is_null(ctx->pMem))
+		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.
@@ -2127,10 +2142,10 @@ static struct sql_func_definition definitions[] = {
 	 FIELD_TYPE_VARBINARY, substrFunc, NULL},
 	{"SUM", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_sum, fin_sum},
 	{"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, 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_INTEGER}, FIELD_TYPE_DOUBLE, step_total,
+	 fin_total},
+	{"TOTAL", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_total,
+	 fin_total},
 
 	{"TRIM", 2, {FIELD_TYPE_STRING, FIELD_TYPE_INTEGER},
 	 FIELD_TYPE_STRING, trim_func, NULL},
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v3 09/15] sql: refactor AVG() function
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (7 preceding siblings ...)
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 08/15] sql: refactor TOTAL() function Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:43 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 10/15] sql: refactor COUNT() function Mergen Imeev via Tarantool-patches
                   ` (5 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:43 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

Part of #4145
---
 src/box/sql/func.c | 119 ++++++++++++++++++++-------------------------
 1 file changed, 54 insertions(+), 65 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index e809ea90c..4ee0937a5 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -102,6 +102,58 @@ 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(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem));
+	if (mem_is_null(argv[0]))
+		return;
+	struct Mem *mem;
+	uint32_t *count;
+	if (mem_is_null(ctx->pMem)) {
+		uint32_t size = sizeof(struct Mem) + sizeof(uint32_t);
+		mem = sqlDbMallocRawNN(sql_get(), size);
+		if (mem == NULL) {
+			ctx->is_aborted = true;
+			return;
+		}
+		count = (uint32_t *)(mem + 1);
+		mem_create(mem);
+		*count = 1;
+		mem_copy_as_ephemeral(mem, argv[0]);
+		mem_set_bin_allocated(ctx->pMem, (char *)mem, size);
+		return;
+	}
+	mem = (struct Mem *)ctx->pMem->z;
+	count = (uint32_t *)(mem + 1);
+	++*count;
+	if (mem_add(mem, argv[0], mem) != 0)
+		ctx->is_aborted = true;
+}
+
+/** Finalizer for the AVG() function. */
+static void
+fin_avg(struct sql_context *ctx)
+{
+	assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem));
+	if (mem_is_null(ctx->pMem))
+		return mem_set_null(ctx->pOut);
+	struct Mem *tmp = (struct Mem *)ctx->pMem->z;
+	uint32_t *count_val = (uint32_t *)(tmp + 1);
+	struct Mem sum;
+	mem_create(&sum);
+	mem_copy_as_ephemeral(&sum, tmp);
+	mem_destroy(tmp);
+	struct Mem count;
+	mem_create(&count);
+	mem_set_uint(&count, *count_val);
+	if (mem_div(&sum, &count, ctx->pOut) != 0)
+		ctx->is_aborted = true;
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1656,69 +1708,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 +2004,8 @@ 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_INTEGER}, FIELD_TYPE_INTEGER, step_avg, fin_avg},
+	{"AVG", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, 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},
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v3 10/15] sql: refactor COUNT() function
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (8 preceding siblings ...)
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 09/15] sql: refactor AVG() function Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:43 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 11/15] sql: refactor MIN() and MAX() functions Mergen Imeev via Tarantool-patches
                   ` (4 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:43 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

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 4ee0937a5..6c4f80243 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -154,6 +154,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 (mem_is_null(ctx->pMem))
+		mem_set_uint(ctx->pMem, 0);
+	if (argc == 1 && mem_is_null(argv[0]))
+		return;
+	assert(mem_is_uint(ctx->pMem));
+	++ctx->pMem->u.u;
+}
+
+/** Finalizer for the COUNT() function. */
+static void
+fin_count(struct sql_context *ctx)
+{
+	assert(mem_is_null(ctx->pMem) || mem_is_uint(ctx->pMem));
+	if (mem_is_null(ctx->pMem))
+		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)
 {
@@ -1708,41 +1731,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.
  */
@@ -2011,9 +1999,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] 16+ messages in thread

* [Tarantool-patches] [PATCH v3 11/15] sql: refactor MIN() and MAX() functions
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (9 preceding siblings ...)
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 10/15] sql: refactor COUNT() function Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:43 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 12/15] sql: refactor GROUP_CONCAT() function Mergen Imeev via Tarantool-patches
                   ` (3 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:43 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

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 6c4f80243..5c6d38d08 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -177,6 +177,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 (mem_is_null(argv[0])) {
+		if (!mem_is_null(ctx->pMem))
+			ctx->skipFlag = 1;
+		return;
+	}
+	if (mem_is_null(ctx->pMem)) {
+		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)
 {
@@ -224,16 +264,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
  */
@@ -1731,60 +1761,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?)
  */
@@ -2055,35 +2031,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 66ac2c4f3..ee07af9c2 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] 16+ messages in thread

* [Tarantool-patches] [PATCH v3 12/15] sql: refactor GROUP_CONCAT() function
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (10 preceding siblings ...)
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 11/15] sql: refactor MIN() and MAX() functions Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:43 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 13/15] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:43 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch does some refactoring for the GROUP_CONCAT() function. He
also adds one test to make sure the behavior hasn't changed after
refactoring.

Part of #4145
---
 src/box/sql/func.c         | 130 +++++++++++++++++--------------------
 test/sql-tap/func.test.lua |  15 +++--
 2 files changed, 68 insertions(+), 77 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 5c6d38d08..93181e89c 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -217,6 +217,61 @@ 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 (mem_is_null(argv[0]))
+		return;
+	assert(mem_is_str(argv[0]) || mem_is_bin(argv[0]));
+	if (mem_is_null(ctx->pMem)) {
+		if (mem_copy(ctx->pMem, argv[0]) != 0)
+			ctx->is_aborted = true;
+		return;
+	}
+	assert(!mem_is_zerobin(ctx->pMem));
+	const char *sep = NULL;
+	int sep_len = 0;
+	if (argc == 1) {
+		sep = ",";
+		sep_len = 1;
+	} else if (mem_is_null(argv[1])) {
+		sep = "";
+		sep_len = 0;
+	} else {
+		assert(mem_is_same_type(argv[0], argv[1]));
+		sep = argv[1]->z;
+		sep_len = argv[1]->n;
+	}
+	if (mem_append(ctx->pMem, sep, sep_len) != 0) {
+		ctx->is_aborted = true;
+		return;
+	}
+	uint32_t size;
+	char *str;
+	if (mem_is_zerobin(argv[0])) {
+		size = argv[0]->u.nZero;
+		str = sqlDbMallocRawNN(sql_get(), size);
+		memset(str, 0, size);
+	} else {
+		size = argv[0]->n;
+		str = argv[0]->z;
+	}
+	if (mem_append(ctx->pMem, str, size) != 0)
+		ctx->is_aborted = true;
+	if (mem_is_zerobin(argv[0]))
+		sqlDbFree(sql_get(), str);
+}
+
+/** 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)
 {
@@ -1761,73 +1816,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)
 {
@@ -1994,13 +1982,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,
diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index 416f27d69..bd8a8fe78 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(14680)
+test:plan(14681)
 
 --!./tcltestrunner.lua
 -- 2001 September 15
@@ -2142,11 +2142,14 @@ test:do_execsql_test(
         -- </func-24.2>
     })
 
--- do_test func-24.3 {
---   execsql {
---     SELECT group_concat(t1,' ' || rowid || ' ') FROM tbl1
---   }
--- } {{this 2 program 3 is 4 free 5 software}}
+test:do_execsql_test(
+    "func-24.3",
+    [[
+        SELECT group_concat(zeroblob(10));
+    ]], {
+        '\0\0\0\0\0\0\0\0\0\0'
+    })
+
 test:do_execsql_test(
     "func-24.4",
     [[
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v3 13/15] sql: remove copying of result in finalizers
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (11 preceding siblings ...)
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 12/15] sql: refactor GROUP_CONCAT() function Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:43 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 14/15] sql: remove MEM_TYPE_AGG Mergen Imeev via Tarantool-patches
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 15/15] sql: remove field argv from struct sql_context Mergen Imeev via Tarantool-patches
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:43 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    | 174 +++++++++++++++++-------------------------
 src/box/sql/sqlInt.h  |  14 +---
 src/box/sql/vdbe.c    |  28 +------
 src/box/sql/vdbeInt.h |   1 -
 src/box/sql/vdbeapi.c |  39 ----------
 5 files changed, 75 insertions(+), 181 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 93181e89c..6f6908566 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -59,47 +59,38 @@ step_sum(struct sql_context *ctx, int argc, struct Mem **argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	assert(mem_is_null(ctx->pMem) || mem_is_num(ctx->pMem));
+	assert(mem_is_null(ctx->pOut) || mem_is_num(ctx->pOut));
 	if (mem_is_null(argv[0]))
 		return;
-	if (mem_is_null(ctx->pMem))
-		return mem_copy_as_ephemeral(ctx->pMem, argv[0]);
-	if (mem_add(ctx->pMem, argv[0], ctx->pMem) != 0)
+	if (mem_is_null(ctx->pOut))
+		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(mem_is_null(ctx->pMem) || 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(mem_is_null(ctx->pMem) || mem_is_num(ctx->pMem));
+	assert(mem_is_null(ctx->pOut) || mem_is_num(ctx->pOut));
 	if (mem_is_null(argv[0]))
 		return;
-	if (mem_is_null(ctx->pMem))
-		mem_set_double(ctx->pMem, 0.0);
-	if (mem_add(ctx->pMem, argv[0], ctx->pMem) != 0)
+	if (mem_is_null(ctx->pOut))
+		mem_set_double(ctx->pOut, 0.0);
+	if (mem_add(ctx->pOut, argv[0], ctx->pOut) != 0)
 		ctx->is_aborted = true;
 }
 
 /** Finalizer for the TOTAL() function. */
-static void
-fin_total(struct sql_context *ctx)
+static int
+fin_total(struct Mem *mem)
 {
-	assert(mem_is_null(ctx->pMem) || mem_is_double(ctx->pMem));
-	if (mem_is_null(ctx->pMem))
-		mem_set_double(ctx->pOut, 0.0);
-	else
-		mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
+	assert(mem_is_null(mem) || mem_is_double(mem));
+	if (mem_is_null(mem))
+		mem_set_double(mem, 0.0);
+	return 0;
 }
 
 /** Implementation of the AVG() function. */
@@ -108,12 +99,12 @@ step_avg(struct sql_context *ctx, int argc, struct Mem **argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem));
+	assert(mem_is_null(ctx->pOut) || mem_is_bin(ctx->pOut));
 	if (mem_is_null(argv[0]))
 		return;
 	struct Mem *mem;
 	uint32_t *count;
-	if (mem_is_null(ctx->pMem)) {
+	if (mem_is_null(ctx->pOut)) {
 		uint32_t size = sizeof(struct Mem) + sizeof(uint32_t);
 		mem = sqlDbMallocRawNN(sql_get(), size);
 		if (mem == NULL) {
@@ -124,10 +115,10 @@ step_avg(struct sql_context *ctx, int argc, struct Mem **argv)
 		mem_create(mem);
 		*count = 1;
 		mem_copy_as_ephemeral(mem, argv[0]);
-		mem_set_bin_allocated(ctx->pMem, (char *)mem, size);
+		mem_set_bin_allocated(ctx->pOut, (char *)mem, size);
 		return;
 	}
-	mem = (struct Mem *)ctx->pMem->z;
+	mem = (struct Mem *)ctx->pOut->z;
 	count = (uint32_t *)(mem + 1);
 	++*count;
 	if (mem_add(mem, argv[0], mem) != 0)
@@ -135,13 +126,15 @@ step_avg(struct sql_context *ctx, int argc, struct Mem **argv)
 }
 
 /** Finalizer for the AVG() function. */
-static void
-fin_avg(struct sql_context *ctx)
+static int
+fin_avg(struct Mem *mem)
 {
-	assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem));
-	if (mem_is_null(ctx->pMem))
-		return mem_set_null(ctx->pOut);
-	struct Mem *tmp = (struct Mem *)ctx->pMem->z;
+	assert(mem_is_null(mem) || mem_is_bin(mem));
+	if (mem_is_null(mem)) {
+		mem_set_null(mem);
+		return 0;
+	}
+	struct Mem *tmp = (struct Mem *)mem->z;
 	uint32_t *count_val = (uint32_t *)(tmp + 1);
 	struct Mem sum;
 	mem_create(&sum);
@@ -150,8 +143,7 @@ fin_avg(struct sql_context *ctx)
 	struct Mem count;
 	mem_create(&count);
 	mem_set_uint(&count, *count_val);
-	if (mem_div(&sum, &count, ctx->pOut) != 0)
-		ctx->is_aborted = true;
+	return mem_div(&sum, &count, mem);
 }
 
 /** Implementation of the COUNT() function. */
@@ -159,22 +151,22 @@ static void
 step_count(struct sql_context *ctx, int argc, struct Mem **argv)
 {
 	assert(argc == 0 || argc == 1);
-	if (mem_is_null(ctx->pMem))
-		mem_set_uint(ctx->pMem, 0);
+	if (mem_is_null(ctx->pOut))
+		mem_set_uint(ctx->pOut, 0);
 	if (argc == 1 && mem_is_null(argv[0]))
 		return;
-	assert(mem_is_uint(ctx->pMem));
-	++ctx->pMem->u.u;
+	assert(mem_is_uint(ctx->pOut));
+	++ctx->pOut->u.u;
 }
 
 /** Finalizer for the COUNT() function. */
-static void
-fin_count(struct sql_context *ctx)
+static int
+fin_count(struct Mem *mem)
 {
-	assert(mem_is_null(ctx->pMem) || mem_is_uint(ctx->pMem));
-	if (mem_is_null(ctx->pMem))
-		return mem_set_uint(ctx->pOut, 0);
-	mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
+	assert(mem_is_null(mem) || mem_is_uint(mem));
+	if (mem_is_null(mem))
+		mem_set_uint(mem, 0);
+	return 0;
 }
 
 /** Implementation of the MIN() and MAX() functions. */
@@ -184,12 +176,12 @@ step_minmax(struct sql_context *ctx, int argc, struct Mem **argv)
 	assert(argc == 1);
 	(void)argc;
 	if (mem_is_null(argv[0])) {
-		if (!mem_is_null(ctx->pMem))
+		if (!mem_is_null(ctx->pOut))
 			ctx->skipFlag = 1;
 		return;
 	}
-	if (mem_is_null(ctx->pMem)) {
-		if (mem_copy(ctx->pMem, argv[0]) != 0)
+	if (mem_is_null(ctx->pOut)) {
+		if (mem_copy(ctx->pOut, argv[0]) != 0)
 			ctx->is_aborted = true;
 		return;
 	}
@@ -201,22 +193,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)
@@ -226,12 +211,12 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
 	if (mem_is_null(argv[0]))
 		return;
 	assert(mem_is_str(argv[0]) || mem_is_bin(argv[0]));
-	if (mem_is_null(ctx->pMem)) {
-		if (mem_copy(ctx->pMem, argv[0]) != 0)
+	if (mem_is_null(ctx->pOut)) {
+		if (mem_copy(ctx->pOut, argv[0]) != 0)
 			ctx->is_aborted = true;
 		return;
 	}
-	assert(!mem_is_zerobin(ctx->pMem));
+	assert(!mem_is_zerobin(ctx->pOut));
 	const char *sep = NULL;
 	int sep_len = 0;
 	if (argc == 1) {
@@ -245,7 +230,7 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
 		sep = argv[1]->z;
 		sep_len = argv[1]->n;
 	}
-	if (mem_append(ctx->pMem, sep, sep_len) != 0) {
+	if (mem_append(ctx->pOut, sep, sep_len) != 0) {
 		ctx->is_aborted = true;
 		return;
 	}
@@ -259,19 +244,12 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
 		size = argv[0]->n;
 		str = argv[0]->z;
 	}
-	if (mem_append(ctx->pMem, str, size) != 0)
+	if (mem_append(ctx->pOut, str, size) != 0)
 		ctx->is_aborted = true;
 	if (mem_is_zerobin(argv[0]))
 		sqlDbFree(sql_get(), str);
 }
 
-/** 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)
 {
@@ -1946,7 +1924,7 @@ struct sql_func_definition {
 	/** Call implementation with given arguments. */
 	void (*call)(sql_context *ctx, int argc, sql_value **argv);
 	/** Call finalization function for this implementation. */
-	void (*finalize)(sql_context *ctx);
+	int (*finalize)(struct Mem *mem);
 };
 
 /**
@@ -1982,13 +1960,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,
@@ -2019,35 +1997,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},
@@ -2081,8 +2047,8 @@ 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, step_sum, fin_sum},
-	{"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_sum, fin_sum},
+	{"SUM", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_sum, NULL},
+	{"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_sum, NULL},
 	{"TOTAL", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_DOUBLE, step_total,
 	 fin_total},
 	{"TOTAL", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_total,
@@ -2333,7 +2299,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 abcc3bfdf..5bce70eaa 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);
 
@@ -4386,7 +4374,7 @@ struct func_sql_builtin {
 	 * A VDBE-memory-compatible finalize method
 	 * (is valid only for aggregate function).
 	 */
-	void (*finalize)(sql_context *ctx);
+	int (*finalize)(struct Mem *mem);
 };
 
 /**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index ee07af9c2..5dd68912c 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,20 +4200,7 @@ 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;
-	mem_create(&t);
-	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)
+	if (func->finalize != NULL && func->finalize(pIn1) != 0)
 		goto abort_due_to_error;
 	UPDATE_MAX_BLOBSIZE(pIn1);
 	if (sqlVdbeMemTooBig(pIn1) != 0)
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] 16+ messages in thread

* [Tarantool-patches] [PATCH v3 14/15] sql: remove MEM_TYPE_AGG
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (12 preceding siblings ...)
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 13/15] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:43 ` Mergen Imeev via Tarantool-patches
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 15/15] sql: remove field argv from struct sql_context Mergen Imeev via Tarantool-patches
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:43 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 e59935672..3af770f48 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] 16+ messages in thread

* [Tarantool-patches] [PATCH v3 15/15] sql: remove field argv from struct sql_context
  2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
                   ` (13 preceding siblings ...)
  2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 14/15] sql: remove MEM_TYPE_AGG Mergen Imeev via Tarantool-patches
@ 2021-10-01  7:43 ` Mergen Imeev via Tarantool-patches
  14 siblings, 0 replies; 16+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-10-01  7:43 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

Since the function arguments are always sequential in the array of all
VDBE MEMs, we don't need to store the position of each argument. The
position of the first MEM and the number of arguments are sufficient to
describe all the arguments.

Part of #4145
---
 src/box/sql/expr.c    |   2 +-
 src/box/sql/func.c    | 325 ++++++++++++++++++++++--------------------
 src/box/sql/main.c    |   5 +-
 src/box/sql/select.c  |   9 +-
 src/box/sql/sqlInt.h  |   7 +-
 src/box/sql/vdbe.c    |  40 ++----
 src/box/sql/vdbeInt.h |   1 -
 7 files changed, 188 insertions(+), 201 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index ab7d95f7e..db8355f33 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4104,7 +4104,7 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			}
 			if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
 				struct sql_context *ctx =
-					sql_context_new(func, nFarg, coll);
+					sql_context_new(func, 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 6f6908566..2010b3cf6 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -55,31 +55,31 @@ static struct func_sql_builtin **functions;
 
 /** Implementation of the SUM() function. */
 static void
-step_sum(struct sql_context *ctx, int argc, struct Mem **argv)
+step_sum(struct sql_context *ctx, int argc, struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
 	assert(mem_is_null(ctx->pOut) || mem_is_num(ctx->pOut));
-	if (mem_is_null(argv[0]))
+	if (mem_is_null(&argv[0]))
 		return;
 	if (mem_is_null(ctx->pOut))
-		return mem_copy_as_ephemeral(ctx->pOut, argv[0]);
-	if (mem_add(ctx->pOut, argv[0], ctx->pOut) != 0)
+		return mem_copy_as_ephemeral(ctx->pOut, &argv[0]);
+	if (mem_add(ctx->pOut, &argv[0], ctx->pOut) != 0)
 		ctx->is_aborted = true;
 }
 
 /** Implementation of the TOTAL() function. */
 static void
-step_total(struct sql_context *ctx, int argc, struct Mem **argv)
+step_total(struct sql_context *ctx, int argc, struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
 	assert(mem_is_null(ctx->pOut) || mem_is_num(ctx->pOut));
-	if (mem_is_null(argv[0]))
+	if (mem_is_null(&argv[0]))
 		return;
 	if (mem_is_null(ctx->pOut))
 		mem_set_double(ctx->pOut, 0.0);
-	if (mem_add(ctx->pOut, argv[0], ctx->pOut) != 0)
+	if (mem_add(ctx->pOut, &argv[0], ctx->pOut) != 0)
 		ctx->is_aborted = true;
 }
 
@@ -95,12 +95,12 @@ fin_total(struct Mem *mem)
 
 /** Implementation of the AVG() function. */
 static void
-step_avg(struct sql_context *ctx, int argc, struct Mem **argv)
+step_avg(struct sql_context *ctx, int argc, struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
 	assert(mem_is_null(ctx->pOut) || mem_is_bin(ctx->pOut));
-	if (mem_is_null(argv[0]))
+	if (mem_is_null(&argv[0]))
 		return;
 	struct Mem *mem;
 	uint32_t *count;
@@ -114,14 +114,14 @@ step_avg(struct sql_context *ctx, int argc, struct Mem **argv)
 		count = (uint32_t *)(mem + 1);
 		mem_create(mem);
 		*count = 1;
-		mem_copy_as_ephemeral(mem, argv[0]);
+		mem_copy_as_ephemeral(mem, &argv[0]);
 		mem_set_bin_allocated(ctx->pOut, (char *)mem, size);
 		return;
 	}
 	mem = (struct Mem *)ctx->pOut->z;
 	count = (uint32_t *)(mem + 1);
 	++*count;
-	if (mem_add(mem, argv[0], mem) != 0)
+	if (mem_add(mem, &argv[0], mem) != 0)
 		ctx->is_aborted = true;
 }
 
@@ -148,12 +148,12 @@ fin_avg(struct Mem *mem)
 
 /** Implementation of the COUNT() function. */
 static void
-step_count(struct sql_context *ctx, int argc, struct Mem **argv)
+step_count(struct sql_context *ctx, int argc, struct Mem *argv)
 {
 	assert(argc == 0 || argc == 1);
 	if (mem_is_null(ctx->pOut))
 		mem_set_uint(ctx->pOut, 0);
-	if (argc == 1 && mem_is_null(argv[0]))
+	if (argc == 1 && mem_is_null(&argv[0]))
 		return;
 	assert(mem_is_uint(ctx->pOut));
 	++ctx->pOut->u.u;
@@ -171,17 +171,17 @@ fin_count(struct Mem *mem)
 
 /** Implementation of the MIN() and MAX() functions. */
 static void
-step_minmax(struct sql_context *ctx, int argc, struct Mem **argv)
+step_minmax(struct sql_context *ctx, int argc, struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	if (mem_is_null(argv[0])) {
+	if (mem_is_null(&argv[0])) {
 		if (!mem_is_null(ctx->pOut))
 			ctx->skipFlag = 1;
 		return;
 	}
 	if (mem_is_null(ctx->pOut)) {
-		if (mem_copy(ctx->pOut, argv[0]) != 0)
+		if (mem_copy(ctx->pOut, &argv[0]) != 0)
 			ctx->is_aborted = true;
 		return;
 	}
@@ -193,9 +193,9 @@ 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->pOut, 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->pOut, argv[0]) != 0)
+		if (mem_copy(ctx->pOut, &argv[0]) != 0)
 			ctx->is_aborted = true;
 		return;
 	}
@@ -204,15 +204,15 @@ step_minmax(struct sql_context *ctx, int argc, struct Mem **argv)
 
 /** Implementation of the GROUP_CONCAT() function. */
 static void
-step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
+step_group_concat(struct sql_context *ctx, int argc, struct Mem *argv)
 {
 	assert(argc == 1 || argc == 2);
 	(void)argc;
-	if (mem_is_null(argv[0]))
+	if (mem_is_null(&argv[0]))
 		return;
-	assert(mem_is_str(argv[0]) || mem_is_bin(argv[0]));
+	assert(mem_is_str(&argv[0]) || mem_is_bin(&argv[0]));
 	if (mem_is_null(ctx->pOut)) {
-		if (mem_copy(ctx->pOut, argv[0]) != 0)
+		if (mem_copy(ctx->pOut, &argv[0]) != 0)
 			ctx->is_aborted = true;
 		return;
 	}
@@ -222,13 +222,13 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
 	if (argc == 1) {
 		sep = ",";
 		sep_len = 1;
-	} else if (mem_is_null(argv[1])) {
+	} else if (mem_is_null(&argv[1])) {
 		sep = "";
 		sep_len = 0;
 	} else {
-		assert(mem_is_same_type(argv[0], argv[1]));
-		sep = argv[1]->z;
-		sep_len = argv[1]->n;
+		assert(mem_is_same_type(&argv[0], &argv[1]));
+		sep = argv[1].z;
+		sep_len = argv[1].n;
 	}
 	if (mem_append(ctx->pOut, sep, sep_len) != 0) {
 		ctx->is_aborted = true;
@@ -236,17 +236,17 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
 	}
 	uint32_t size;
 	char *str;
-	if (mem_is_zerobin(argv[0])) {
-		size = argv[0]->u.nZero;
+	if (mem_is_zerobin(&argv[0])) {
+		size = argv[0].u.nZero;
 		str = sqlDbMallocRawNN(sql_get(), size);
 		memset(str, 0, size);
 	} else {
-		size = argv[0]->n;
-		str = argv[0]->z;
+		size = argv[0].n;
+		str = argv[0].z;
 	}
 	if (mem_append(ctx->pOut, str, size) != 0)
 		ctx->is_aborted = true;
-	if (mem_is_zerobin(argv[0]))
+	if (mem_is_zerobin(&argv[0]))
 		sqlDbFree(sql_get(), str);
 }
 
@@ -269,7 +269,7 @@ mem_as_bin(struct Mem *mem)
 }
 
 static void
-sql_func_uuid(struct sql_context *ctx, int argc, struct Mem **argv)
+sql_func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
 {
 	if (argc > 1) {
 		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "UUID",
@@ -279,9 +279,9 @@ sql_func_uuid(struct sql_context *ctx, int argc, struct Mem **argv)
 	}
 	if (argc == 1) {
 		uint64_t version;
-		if (mem_get_uint(argv[0], &version) != 0) {
+		if (mem_get_uint(&argv[0], &version) != 0) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-				 mem_str(argv[0]), "integer");
+				 mem_str(&argv[0]), "integer");
 			ctx->is_aborted = true;
 			return;
 		}
@@ -301,7 +301,7 @@ sql_func_uuid(struct sql_context *ctx, int argc, struct Mem **argv)
  * Implementation of the non-aggregate min() and max() functions
  */
 static void
-minmaxFunc(sql_context * context, int argc, sql_value ** argv)
+minmaxFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	int i;
 	int iBest;
@@ -317,30 +317,30 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
 	pColl = context->coll;
 	assert(mask == -1 || mask == 0);
 	iBest = 0;
-	if (mem_is_null(argv[0]))
+	if (mem_is_null(&argv[0]))
 		return;
 	for (i = 1; i < argc; i++) {
-		if (mem_is_null(argv[i]))
+		if (mem_is_null(&argv[i]))
 			return;
-		if ((mem_cmp_scalar(argv[iBest], argv[i], pColl) ^ mask) >= 0)
+		if ((mem_cmp_scalar(&argv[iBest], &argv[i], pColl) ^ mask) >= 0)
 			iBest = i;
 	}
-	sql_result_value(context, argv[iBest]);
+	sql_result_value(context, &argv[iBest]);
 }
 
 /*
  * Return the type of the argument.
  */
 static void
-typeofFunc(sql_context * context, int NotUsed, sql_value ** argv)
+typeofFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
+	(void)argc;
 	const char *z = 0;
-	UNUSED_PARAMETER(NotUsed);
-	if ((argv[0]->flags & MEM_Number) != 0)
+	if ((argv[0].flags & MEM_Number) != 0)
 		return mem_set_str0_static(context->pOut, "number");
-	if ((argv[0]->flags & MEM_Scalar) != 0)
+	if ((argv[0].flags & MEM_Scalar) != 0)
 		return mem_set_str0_static(context->pOut, "scalar");
-	switch (argv[0]->type) {
+	switch (argv[0].type) {
 	case MEM_TYPE_INT:
 	case MEM_TYPE_UINT:
 		z = "integer";
@@ -379,13 +379,13 @@ typeofFunc(sql_context * context, int NotUsed, sql_value ** argv)
  * Implementation of the length() function
  */
 static void
-lengthFunc(sql_context * context, int argc, sql_value ** argv)
+lengthFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	int len;
 
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
-	switch (sql_value_type(argv[0])) {
+	switch (sql_value_type(&argv[0])) {
 	case MP_BIN:
 	case MP_ARRAY:
 	case MP_MAP:
@@ -393,16 +393,16 @@ lengthFunc(sql_context * context, int argc, sql_value ** argv)
 	case MP_UINT:
 	case MP_BOOL:
 	case MP_DOUBLE:{
-			mem_as_bin(argv[0]);
-			sql_result_uint(context, mem_len_unsafe(argv[0]));
+			mem_as_bin(&argv[0]);
+			sql_result_uint(context, mem_len_unsafe(&argv[0]));
 			break;
 		}
 	case MP_EXT:
 	case MP_STR:{
-			const unsigned char *z = mem_as_ustr(argv[0]);
+			const unsigned char *z = mem_as_ustr(&argv[0]);
 			if (z == 0)
 				return;
-			len = sql_utf8_char_count(z, mem_len_unsafe(argv[0]));
+			len = sql_utf8_char_count(z, mem_len_unsafe(&argv[0]));
 			sql_result_uint(context, len);
 			break;
 		}
@@ -420,17 +420,17 @@ lengthFunc(sql_context * context, int argc, sql_value ** argv)
  * the numeric argument X.
  */
 static void
-absFunc(sql_context * context, int argc, sql_value ** argv)
+absFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
-	switch (sql_value_type(argv[0])) {
+	switch (sql_value_type(&argv[0])) {
 	case MP_UINT: {
-		sql_result_uint(context, mem_get_uint_unsafe(argv[0]));
+		sql_result_uint(context, mem_get_uint_unsafe(&argv[0]));
 		break;
 	}
 	case MP_INT: {
-		int64_t value = mem_get_int_unsafe(argv[0]);
+		int64_t value = mem_get_int_unsafe(&argv[0]);
 		assert(value < 0);
 		sql_result_uint(context, -value);
 		break;
@@ -446,7 +446,7 @@ absFunc(sql_context * context, int argc, sql_value ** argv)
 	case MP_ARRAY:
 	case MP_MAP: {
 		diag_set(ClientError, ER_INCONSISTENT_TYPES, "number",
-			 mem_str(argv[0]));
+			 mem_str(&argv[0]));
 		context->is_aborted = true;
 		return;
 	}
@@ -455,7 +455,7 @@ absFunc(sql_context * context, int argc, sql_value ** argv)
 			 * Abs(X) returns 0.0 if X is a string or blob
 			 * that cannot be converted to a numeric value.
 			 */
-			double rVal = mem_get_double_unsafe(argv[0]);
+			double rVal = mem_get_double_unsafe(&argv[0]);
 			if (rVal < 0)
 				rVal = -rVal;
 			sql_result_double(context, rVal);
@@ -476,11 +476,11 @@ absFunc(sql_context * context, int argc, sql_value ** argv)
  * occurrence of needle, or 0 if needle never occurs in haystack.
  */
 static void
-position_func(struct sql_context *context, int argc, struct Mem **argv)
+position_func(struct sql_context *context, int argc, struct Mem *argv)
 {
 	UNUSED_PARAMETER(argc);
-	struct Mem *needle = argv[0];
-	struct Mem *haystack = argv[1];
+	struct Mem *needle = &argv[0];
+	struct Mem *haystack = &argv[1];
 	enum mp_type needle_type = sql_value_type(needle);
 	enum mp_type haystack_type = sql_value_type(haystack);
 
@@ -603,7 +603,7 @@ finish:
  * Implementation of the printf() function.
  */
 static void
-printfFunc(sql_context * context, int argc, sql_value ** argv)
+printfFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	PrintfArguments x;
 	StrAccum str;
@@ -611,14 +611,22 @@ printfFunc(sql_context * context, int argc, sql_value ** argv)
 	int n;
 	sql *db = sql_context_db_handle(context);
 
-	if (argc >= 1 && (zFormat = mem_as_str0(argv[0])) != NULL) {
+	if (argc >= 1 && (zFormat = mem_as_str0(&argv[0])) != NULL) {
 		x.nArg = argc - 1;
 		x.nUsed = 0;
-		x.apArg = argv + 1;
+		x.apArg = sqlDbMallocRawNN(sql_get(),
+					   (argc - 1) * sizeof(*x.apArg));
+		if (x.apArg == NULL) {
+			context->is_aborted = true;
+			return;
+		}
+		for (int i = 1; i < argc; ++i)
+			x.apArg[i - 1] = &argv[i];
 		sqlStrAccumInit(&str, db, 0, 0,
 				    db->aLimit[SQL_LIMIT_LENGTH]);
 		str.printfFlags = SQL_PRINTF_SQLFUNC;
 		sqlXPrintf(&str, zFormat, &x);
+		sqlDbFree(sql_get(), x.apArg);
 		n = str.nChar;
 		sql_result_text(context, sqlStrAccumFinish(&str), n,
 				    SQL_DYNAMIC);
@@ -638,7 +646,7 @@ printfFunc(sql_context * context, int argc, sql_value ** argv)
  * If p2 is negative, return the p2 characters preceding p1.
  */
 static void
-substrFunc(sql_context * context, int argc, sql_value ** argv)
+substrFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	const unsigned char *z;
 	const unsigned char *z2;
@@ -653,26 +661,26 @@ substrFunc(sql_context * context, int argc, sql_value ** argv)
 		context->is_aborted = true;
 		return;
 	}
-	if (mem_is_null(argv[1]) || (argc == 3 && mem_is_null(argv[2])))
+	if (mem_is_null(&argv[1]) || (argc == 3 && mem_is_null(&argv[2])))
 		return;
-	p0type = sql_value_type(argv[0]);
-	p1 = mem_get_int_unsafe(argv[1]);
+	p0type = sql_value_type(&argv[0]);
+	p1 = mem_get_int_unsafe(&argv[1]);
 	if (p0type == MP_BIN) {
-		z = mem_as_bin(argv[0]);
-		len = mem_len_unsafe(argv[0]);
+		z = mem_as_bin(&argv[0]);
+		len = mem_len_unsafe(&argv[0]);
 		if (z == 0)
 			return;
-		assert(len == mem_len_unsafe(argv[0]));
+		assert(len == mem_len_unsafe(&argv[0]));
 	} else {
-		z = mem_as_ustr(argv[0]);
+		z = mem_as_ustr(&argv[0]);
 		if (z == 0)
 			return;
 		len = 0;
 		if (p1 < 0)
-			len = sql_utf8_char_count(z, mem_len_unsafe(argv[0]));
+			len = sql_utf8_char_count(z, mem_len_unsafe(&argv[0]));
 	}
 	if (argc == 3) {
-		p2 = mem_get_int_unsafe(argv[2]);
+		p2 = mem_get_int_unsafe(&argv[2]);
 		if (p2 < 0) {
 			p2 = -p2;
 			negP2 = 1;
@@ -708,7 +716,7 @@ substrFunc(sql_context * context, int argc, sql_value ** argv)
 		 * used because '\0' is not supposed to be
 		 * end-of-string symbol.
 		 */
-		int byte_size = mem_len_unsafe(argv[0]);
+		int byte_size = mem_len_unsafe(&argv[0]);
 		int n_chars = sql_utf8_char_count(z, byte_size);
 		int cnt = 0;
 		int i = 0;
@@ -739,7 +747,7 @@ substrFunc(sql_context * context, int argc, sql_value ** argv)
  * Implementation of the round() function
  */
 static void
-roundFunc(sql_context * context, int argc, sql_value ** argv)
+roundFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	int64_t n = 0;
 	double r;
@@ -750,21 +758,21 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
 		return;
 	}
 	if (argc == 2) {
-		if (mem_is_null(argv[1]))
+		if (mem_is_null(&argv[1]))
 			return;
-		n = mem_get_int_unsafe(argv[1]);
+		n = mem_get_int_unsafe(&argv[1]);
 		if (n < 0)
 			n = 0;
 	}
-	if (mem_is_null(argv[0]))
+	if (mem_is_null(&argv[0]))
 		return;
-	if (!mem_is_num(argv[0]) && !mem_is_str(argv[0])) {
+	if (!mem_is_num(&argv[0]) && !mem_is_str(&argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 mem_str(argv[0]), "number");
+			 mem_str(&argv[0]), "number");
 		context->is_aborted = true;
 		return;
 	}
-	r = mem_get_double_unsafe(argv[0]);
+	r = mem_get_double_unsafe(&argv[0]);
 	/* If Y==0 and X will fit in a 64-bit int,
 	 * handle the rounding directly,
 	 * otherwise use printf.
@@ -787,7 +795,7 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
  * NULL.
  */
 static void *
-contextMalloc(sql_context * context, i64 nByte)
+contextMalloc(struct sql_context *context, i64 nByte)
 {
 	char *z;
 	sql *db = sql_context_db_handle(context);
@@ -812,26 +820,26 @@ contextMalloc(sql_context * context, i64 nByte)
 
 #define ICU_CASE_CONVERT(case_type)                                            \
 static void                                                                    \
-case_type##ICUFunc(sql_context *context, int argc, sql_value **argv)   \
+case_type##ICUFunc(sql_context *context, int argc, struct Mem *argv)           \
 {                                                                              \
 	char *z1;                                                              \
 	const char *z2;                                                        \
 	int n;                                                                 \
 	UNUSED_PARAMETER(argc);                                                \
-	if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) ||                      \
-	    mem_is_array(argv[0])) {                                           \
+	if (mem_is_bin(&argv[0]) || mem_is_map(&argv[0]) ||                    \
+	    mem_is_array(&argv[0])) {                                          \
 		diag_set(ClientError, ER_INCONSISTENT_TYPES, "string",         \
-			 mem_str(argv[0]));                                    \
+			 mem_str(&argv[0]));                                   \
 		context->is_aborted = true;                                    \
 		return;                                                        \
 	}                                                                      \
-	z2 = mem_as_str0(argv[0]);                                             \
-	n = mem_len_unsafe(argv[0]);                                           \
+	z2 = mem_as_str0(&argv[0]);                                            \
+	n = mem_len_unsafe(&argv[0]);                                          \
 	/*                                                                     \
 	 * Verify that the call to _bytes()                                    \
 	 * does not invalidate the _text() pointer.                            \
 	 */                                                                    \
-	assert(z2 == mem_as_str0(argv[0]));                                    \
+	assert(z2 == mem_as_str0(&argv[0]));                                   \
 	if (!z2)                                                               \
 		return;                                                        \
 	z1 = contextMalloc(context, ((i64) n) + 1);                            \
@@ -881,10 +889,11 @@ ICU_CASE_CONVERT(Upper);
  * Implementation of random().  Return a random integer.
  */
 static void
-randomFunc(sql_context * context, int NotUsed, sql_value ** NotUsed2)
+randomFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
+	(void)argc;
+	(void)argv;
 	int64_t r;
-	UNUSED_PARAMETER2(NotUsed, NotUsed2);
 	sql_randomness(sizeof(r), &r);
 	sql_result_int(context, r);
 }
@@ -894,20 +903,20 @@ randomFunc(sql_context * context, int NotUsed, sql_value ** NotUsed2)
  * that is N bytes long.
  */
 static void
-randomBlob(sql_context * context, int argc, sql_value ** argv)
+randomBlob(struct sql_context *context, int argc, struct Mem *argv)
 {
 	int64_t n;
 	unsigned char *p;
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
-	if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) ||
-	    mem_is_array(argv[0])) {
+	if (mem_is_bin(&argv[0]) || mem_is_map(&argv[0]) ||
+	    mem_is_array(&argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 mem_str(argv[0]), "number");
+			 mem_str(&argv[0]), "number");
 		context->is_aborted = true;
 		return;
 	}
-	n = mem_get_int_unsafe(argv[0]);
+	n = mem_get_int_unsafe(&argv[0]);
 	if (n < 1)
 		return;
 	p = contextMalloc(context, n);
@@ -1137,7 +1146,7 @@ sql_utf8_pattern_compare(const char *pattern,
  * is NULL then result is NULL as well.
  */
 static void
-likeFunc(sql_context *context, int argc, sql_value **argv)
+likeFunc(sql_context *context, int argc, struct Mem *argv)
 {
 	u32 escape = SQL_END_OF_STRING;
 	int nPat;
@@ -1148,29 +1157,29 @@ likeFunc(sql_context *context, int argc, sql_value **argv)
 		return;
 	}
 	sql *db = sql_context_db_handle(context);
-	int rhs_type = sql_value_type(argv[0]);
-	int lhs_type = sql_value_type(argv[1]);
+	int rhs_type = sql_value_type(&argv[0]);
+	int lhs_type = sql_value_type(&argv[1]);
 
 	if (lhs_type != MP_STR || rhs_type != MP_STR) {
 		if (lhs_type == MP_NIL || rhs_type == MP_NIL)
 			return;
 		const char *str = rhs_type != MP_STR ?
-				  mem_str(argv[0]) : mem_str(argv[1]);
+				  mem_str(&argv[0]) : mem_str(&argv[1]);
 		diag_set(ClientError, ER_INCONSISTENT_TYPES, "string", str);
 		context->is_aborted = true;
 		return;
 	}
-	const char *zB = mem_as_str0(argv[0]);
-	const char *zA = mem_as_str0(argv[1]);
-	const char *zB_end = zB + mem_len_unsafe(argv[0]);
-	const char *zA_end = zA + mem_len_unsafe(argv[1]);
+	const char *zB = mem_as_str0(&argv[0]);
+	const char *zA = mem_as_str0(&argv[1]);
+	const char *zB_end = zB + mem_len_unsafe(&argv[0]);
+	const char *zA_end = zA + mem_len_unsafe(&argv[1]);
 
 	/*
 	 * Limit the length of the LIKE pattern to avoid problems
 	 * of deep recursion and N*N behavior in
 	 * sql_utf8_pattern_compare().
 	 */
-	nPat = mem_len_unsafe(argv[0]);
+	nPat = mem_len_unsafe(&argv[0]);
 	testcase(nPat == db->aLimit[SQL_LIMIT_LIKE_PATTERN_LENGTH]);
 	testcase(nPat == db->aLimit[SQL_LIMIT_LIKE_PATTERN_LENGTH] + 1);
 	if (nPat > db->aLimit[SQL_LIMIT_LIKE_PATTERN_LENGTH]) {
@@ -1180,7 +1189,7 @@ likeFunc(sql_context *context, int argc, sql_value **argv)
 		return;
 	}
 	/* Encoding did not change */
-	assert(zB == mem_as_str0(argv[0]));
+	assert(zB == mem_as_str0(&argv[0]));
 
 	if (argc == 3) {
 		/*
@@ -1188,10 +1197,10 @@ likeFunc(sql_context *context, int argc, sql_value **argv)
 		 * single UTF-8 character. Otherwise, return an
 		 * error.
 		 */
-		const unsigned char *zEsc = mem_as_ustr(argv[2]);
+		const unsigned char *zEsc = mem_as_ustr(&argv[2]);
 		if (zEsc == 0)
 			return;
-		if (sql_utf8_char_count(zEsc, mem_len_unsafe(argv[2])) != 1) {
+		if (sql_utf8_char_count(zEsc, mem_len_unsafe(&argv[2])) != 1) {
 			diag_set(ClientError, ER_SQL_EXECUTE, "ESCAPE "\
 				 "expression must be a single character");
 			context->is_aborted = true;
@@ -1221,12 +1230,12 @@ likeFunc(sql_context *context, int argc, sql_value **argv)
  * arguments are equal to each other.
  */
 static void
-nullifFunc(sql_context * context, int NotUsed, sql_value ** argv)
+nullifFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
+	(void)argc;
 	struct coll *pColl = context->coll;
-	UNUSED_PARAMETER(NotUsed);
-	if (mem_cmp_scalar(argv[0], argv[1], pColl) != 0)
-		sql_result_value(context, argv[0]);
+	if (mem_cmp_scalar(&argv[0], &argv[1], pColl) != 0)
+		sql_result_value(context, &argv[0]);
 }
 
 /**
@@ -1238,10 +1247,10 @@ nullifFunc(sql_context * context, int NotUsed, sql_value ** argv)
  * @param unused2 Unused.
  */
 static void
-sql_func_version(struct sql_context *context,
-		 MAYBE_UNUSED int unused1,
-		 MAYBE_UNUSED sql_value **unused2)
+sql_func_version(struct sql_context *context, int argc, struct Mem *argv)
 {
+	(void)argc;
+	(void)argv;
 	sql_result_text(context, tarantool_version(), -1, SQL_STATIC);
 }
 
@@ -1261,14 +1270,14 @@ static const char hexdigits[] = {
  * single-quote escapes.
  */
 static void
-quoteFunc(sql_context * context, int argc, sql_value ** argv)
+quoteFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
-	switch (argv[0]->type) {
+	switch (argv[0].type) {
 	case MEM_TYPE_UUID: {
 		char buf[UUID_STR_LEN + 1];
-		tt_uuid_to_string(&argv[0]->u.uuid, &buf[0]);
+		tt_uuid_to_string(&argv[0].u.uuid, &buf[0]);
 		sql_result_text(context, buf, UUID_STR_LEN, SQL_TRANSIENT);
 		break;
 	}
@@ -1276,16 +1285,16 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
 	case MEM_TYPE_DEC:
 	case MEM_TYPE_UINT:
 	case MEM_TYPE_INT: {
-			sql_result_value(context, argv[0]);
+			sql_result_value(context, &argv[0]);
 			break;
 		}
 	case MEM_TYPE_BIN:
 	case MEM_TYPE_ARRAY:
 	case MEM_TYPE_MAP: {
 			char *zText = 0;
-			char const *zBlob = mem_as_bin(argv[0]);
-			int nBlob = mem_len_unsafe(argv[0]);
-			assert(zBlob == mem_as_bin(argv[0]));	/* No encoding change */
+			char const *zBlob = mem_as_bin(&argv[0]);
+			int nBlob = mem_len_unsafe(&argv[0]);
+			assert(zBlob == mem_as_bin(&argv[0]));	/* No encoding change */
 			zText =
 			    (char *)contextMalloc(context,
 						  (2 * (i64) nBlob) + 4);
@@ -1310,7 +1319,7 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
 	case MEM_TYPE_STR: {
 			int i, j;
 			u64 n;
-			const unsigned char *zArg = mem_as_ustr(argv[0]);
+			const unsigned char *zArg = mem_as_ustr(&argv[0]);
 			char *z;
 
 			if (zArg == 0)
@@ -1337,12 +1346,12 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
 		}
 	case MEM_TYPE_BOOL: {
 		sql_result_text(context,
-				SQL_TOKEN_BOOLEAN(mem_get_bool_unsafe(argv[0])),
+				SQL_TOKEN_BOOLEAN(argv[0].u.b),
 				-1, SQL_TRANSIENT);
 		break;
 	}
 	default:{
-			assert(mem_is_null(argv[0]));
+			assert(mem_is_null(&argv[0]));
 			sql_result_text(context, "NULL", 4, SQL_STATIC);
 			break;
 		}
@@ -1354,9 +1363,9 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
  * for the first character of the input string.
  */
 static void
-unicodeFunc(sql_context * context, int argc, sql_value ** argv)
+unicodeFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
-	const unsigned char *z = mem_as_ustr(argv[0]);
+	const unsigned char *z = mem_as_ustr(&argv[0]);
 	(void)argc;
 	if (z && z[0])
 		sql_result_uint(context, sqlUtf8Read(&z));
@@ -1368,7 +1377,7 @@ unicodeFunc(sql_context * context, int argc, sql_value ** argv)
  * is the unicode character for the corresponding integer argument.
  */
 static void
-charFunc(sql_context * context, int argc, sql_value ** argv)
+charFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	unsigned char *z, *zOut;
 	int i;
@@ -1380,10 +1389,10 @@ charFunc(sql_context * context, int argc, sql_value ** argv)
 	for (i = 0; i < argc; i++) {
 		uint64_t x;
 		unsigned c;
-		if (sql_value_type(argv[i]) == MP_INT)
+		if (sql_value_type(&argv[i]) == MP_INT)
 			x = 0xfffd;
 		else
-			x = mem_get_uint_unsafe(argv[i]);
+			x = mem_get_uint_unsafe(&argv[i]);
 		if (x > 0x10ffff)
 			x = 0xfffd;
 		c = (unsigned)(x & 0x1fffff);
@@ -1411,16 +1420,16 @@ charFunc(sql_context * context, int argc, sql_value ** argv)
  * a hexadecimal rendering as text.
  */
 static void
-hexFunc(sql_context * context, int argc, sql_value ** argv)
+hexFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	int i, n;
 	const unsigned char *pBlob;
 	char *zHex, *z;
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
-	pBlob = mem_as_bin(argv[0]);
-	n = mem_len_unsafe(argv[0]);
-	assert(pBlob == mem_as_bin(argv[0]));	/* No encoding change */
+	pBlob = mem_as_bin(&argv[0]);
+	n = mem_len_unsafe(&argv[0]);
+	assert(pBlob == mem_as_bin(&argv[0]));	/* No encoding change */
 	z = zHex = contextMalloc(context, ((i64) n) * 2 + 1);
 	if (zHex) {
 		for (i = 0; i < n; i++, pBlob++) {
@@ -1437,12 +1446,12 @@ hexFunc(sql_context * context, int argc, sql_value ** argv)
  * The zeroblob(N) function returns a zero-filled blob of size N bytes.
  */
 static void
-zeroblobFunc(sql_context * context, int argc, sql_value ** argv)
+zeroblobFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	int64_t n;
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
-	n = mem_get_int_unsafe(argv[0]);
+	n = mem_get_int_unsafe(&argv[0]);
 	if (n < 0)
 		n = 0;
 	if (sql_result_zeroblob64(context, n) != 0) {
@@ -1459,7 +1468,7 @@ zeroblobFunc(sql_context * context, int argc, sql_value ** argv)
  * must be exact.  Collating sequences are not used.
  */
 static void
-replaceFunc(sql_context * context, int argc, sql_value ** argv)
+replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	const unsigned char *zStr;	/* The input string A */
 	const unsigned char *zPattern;	/* The pattern string B */
@@ -1474,29 +1483,29 @@ replaceFunc(sql_context * context, int argc, sql_value ** argv)
 
 	assert(argc == 3);
 	UNUSED_PARAMETER(argc);
-	zStr = mem_as_ustr(argv[0]);
+	zStr = mem_as_ustr(&argv[0]);
 	if (zStr == 0)
 		return;
-	nStr = mem_len_unsafe(argv[0]);
-	assert(zStr == mem_as_ustr(argv[0]));	/* No encoding change */
-	zPattern = mem_as_ustr(argv[1]);
+	nStr = mem_len_unsafe(&argv[0]);
+	assert(zStr == mem_as_ustr(&argv[0]));	/* No encoding change */
+	zPattern = mem_as_ustr(&argv[1]);
 	if (zPattern == 0) {
-		assert(mem_is_null(argv[1])
+		assert(mem_is_null(&argv[1])
 		       || sql_context_db_handle(context)->mallocFailed);
 		return;
 	}
-	nPattern = mem_len_unsafe(argv[1]);
+	nPattern = mem_len_unsafe(&argv[1]);
 	if (nPattern == 0) {
-		assert(!mem_is_null(argv[1]));
-		sql_result_value(context, argv[0]);
+		assert(!mem_is_null(&argv[1]));
+		sql_result_value(context, &argv[0]);
 		return;
 	}
-	assert(zPattern == mem_as_ustr(argv[1]));	/* No encoding change */
-	zRep = mem_as_ustr(argv[2]);
+	assert(zPattern == mem_as_ustr(&argv[1]));	/* No encoding change */
+	zRep = mem_as_ustr(&argv[2]);
 	if (zRep == 0)
 		return;
-	nRep = mem_len_unsafe(argv[2]);
-	assert(zRep == mem_as_ustr(argv[2]));
+	nRep = mem_len_unsafe(&argv[2]);
+	assert(zRep == mem_as_ustr(&argv[2]));
 	nOut = nStr + 1;
 	assert(nOut < SQL_MAX_LENGTH);
 	zOut = contextMalloc(context, (i64) nOut);
@@ -1715,14 +1724,14 @@ trim_func_three_args(struct sql_context *context, sql_value *arg1,
  * implementation depending on the number of arguments.
 */
 static void
-trim_func(struct sql_context *context, int argc, sql_value **argv)
+trim_func(struct sql_context *context, int argc, struct Mem *argv)
 {
 	switch (argc) {
 	case 2:
-		trim_func_two_args(context, argv[0], argv[1]);
+		trim_func_two_args(context, &argv[0], &argv[1]);
 		break;
 	case 3:
-		trim_func_three_args(context, argv[0], argv[1], argv[2]);
+		trim_func_three_args(context, &argv[0], &argv[1], &argv[2]);
 		break;
 	default:
 		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "TRIM",
@@ -1738,7 +1747,7 @@ trim_func(struct sql_context *context, int argc, sql_value **argv)
  * soundex encoding of the string X.
  */
 static void
-soundexFunc(sql_context * context, int argc, sql_value ** argv)
+soundexFunc(struct sql_context *context, int argc, struct Mem *argv)
 {
 	(void) argc;
 	char zResult[8];
@@ -1755,14 +1764,14 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
 		1, 2, 6, 2, 3, 0, 1, 0, 2, 0, 2, 0, 0, 0, 0, 0,
 	};
 	assert(argc == 1);
-	if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) ||
-	    mem_is_array(argv[0])) {
+	if (mem_is_bin(&argv[0]) || mem_is_map(&argv[0]) ||
+	    mem_is_array(&argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 mem_str(argv[0]), "string");
+			 mem_str(&argv[0]), "string");
 		context->is_aborted = true;
 		return;
 	}
-	zIn = (u8 *) mem_as_ustr(argv[0]);
+	zIn = (u8 *) mem_as_ustr(&argv[0]);
 	if (zIn == 0)
 		zIn = (u8 *) "";
 	for (i = 0; zIn[i] && !sqlIsalpha(zIn[i]); i++) {
@@ -1818,7 +1827,7 @@ func_sql_builtin_call_stub(struct func *func, struct port *args,
 }
 
 static void
-sql_builtin_stub(sql_context *ctx, int argc, sql_value **argv)
+sql_builtin_stub(sql_context *ctx, int argc, struct Mem *argv)
 {
 	(void) argc; (void) argv;
 	diag_set(ClientError, ER_SQL_EXECUTE,
@@ -1922,7 +1931,7 @@ struct sql_func_definition {
 	/** Type of the result of the implementation. */
 	enum field_type result;
 	/** Call implementation with given arguments. */
-	void (*call)(sql_context *ctx, int argc, sql_value **argv);
+	void (*call)(sql_context *ctx, int argc, struct Mem *argv);
 	/** Call finalization function for this implementation. */
 	int (*finalize)(struct Mem *mem);
 };
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index b0d32ae32..a4247c760 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -221,9 +221,10 @@ setupLookaside(sql * db, void *pBuf, int sz, int cnt)
 }
 
 void
-sql_row_count(struct sql_context *context, MAYBE_UNUSED int unused1,
-	      MAYBE_UNUSED sql_value **unused2)
+sql_row_count(struct sql_context *context, int argc, struct Mem *argv)
 {
+	(void)argc;
+	(void)argv;
 	sql *db = sql_context_db_handle(context);
 	assert(db->nChange >= 0);
 	sql_result_uint(context, db->nChange);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 6269c2868..8ca967108 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -5638,7 +5638,7 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
 				regHit = ++pParse->nMem;
 			sqlVdbeAddOp1(v, OP_SkipLoad, regHit);
 		}
-		struct sql_context *ctx = sql_context_new(pF->func, nArg, coll);
+		struct sql_context *ctx = sql_context_new(pF->func, coll);
 		if (ctx == NULL) {
 			pParse->is_aborted = true;
 			return;
@@ -6750,12 +6750,9 @@ sql_expr_extract_select(struct Parse *parser, struct Select *select)
 }
 
 struct sql_context *
-sql_context_new(struct func *func, uint32_t argc, struct coll *coll)
+sql_context_new(struct func *func, struct coll *coll)
 {
-	uint32_t size = sizeof(struct sql_context);
-	if (argc > 1)
-		size += (argc - 1) * sizeof(struct Mem *);
-	struct sql_context *ctx = sqlDbMallocRawNN(sql_get(), size);
+	struct sql_context *ctx = sqlDbMallocRawNN(sql_get(), sizeof(*ctx));
 	if (ctx == NULL)
 		return NULL;
 	ctx->pOut = NULL;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 5bce70eaa..7c28c34e1 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -481,8 +481,7 @@ sql_randomness(int N, void *P);
  * Return the number of affected rows in the last SQL statement.
  */
 void
-sql_row_count(struct sql_context *context, MAYBE_UNUSED int unused1,
-	      MAYBE_UNUSED sql_value **unused2);
+sql_row_count(struct sql_context *context, int argc, struct Mem *argv);
 
 int
 sql_column_count(sql_stmt * pStmt);
@@ -4130,7 +4129,7 @@ void sqlStrAccumReset(StrAccum *);
 void sqlSelectDestInit(SelectDest *, int, int, int);
 
 struct sql_context *
-sql_context_new(struct func *func, uint32_t argc, struct coll *coll);
+sql_context_new(struct func *func, struct coll *coll);
 
 void
 sql_context_delete(struct sql_context *ctx);
@@ -4369,7 +4368,7 @@ struct func_sql_builtin {
 	 * Access checks are redundant, because all SQL built-ins
 	 * are predefined and are executed on SQL privilege level.
 	 */
-	void (*call)(sql_context *ctx, int argc, sql_value **argv);
+	void (*call)(struct sql_context *ctx, int argc, struct Mem *argv);
 	/**
 	 * A VDBE-memory-compatible finalize method
 	 * (is valid only for aggregate function).
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 5dd68912c..43ce4fdfc 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1183,35 +1183,26 @@ case OP_SkipLoad: {
  * See also: AggStep, AggFinal
  */
 case OP_BuiltinFunction: {
-	int i;
 	int argc = pOp->p1;
 	sql_context *pCtx;
 
 	assert(pOp->p4type==P4_FUNCCTX);
 	pCtx = pOp->p4.pCtx;
 
-	/* 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
-	 * checks to see if the register array has changed, and if so it
-	 * reinitializes the relavant parts of the sql_context object
-	 */
 	pOut = vdbe_prepare_null_out(p, pOp->p3);
-	if (pCtx->pOut != pOut) {
+	if (pCtx->pOut != pOut)
 		pCtx->pOut = pOut;
-		for(i = 0; i < argc; ++i)
-			pCtx->argv[i] = &aMem[pOp->p2 + i];
-	}
 
 #ifdef SQL_DEBUG
-	for(i = 0; i < argc; i++) {
-		assert(memIsValid(pCtx->argv[i]));
-		REGISTER_TRACE(p, pOp->p2+i, pCtx->argv[i]);
+	for(int i = 0; i < argc; i++) {
+		assert(memIsValid(&aMem[pOp->p2 + i]));
+		REGISTER_TRACE(p, pOp->p2 + i, &aMem[pOp->p2 + i]);
 	}
 #endif
 	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, argc, pCtx->argv);
+	func->call(pCtx, argc, &aMem[pOp->p2]);
 
 	/* If the function returned an error, throw an exception */
 	if (pCtx->is_aborted)
@@ -4141,7 +4132,6 @@ case OP_DecrJumpZero: {      /* jump, in1 */
  * successors.
  */
 case OP_AggStep: {
-	int i;
 	int argc = pOp->p1;
 	sql_context *pCtx;
 	Mem *pMem;
@@ -4150,33 +4140,25 @@ case OP_AggStep: {
 	pCtx = pOp->p4.pCtx;
 	pMem = &aMem[pOp->p3];
 
-	/* 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
-	 * checks to see if the register array has changed, and if so it
-	 * reinitializes the relavant parts of the sql_context object
-	 */
-	if (pCtx->pOut != pMem) {
+	if (pCtx->pOut != pMem)
 		pCtx->pOut = pMem;
-		for(i = 0; i < argc; ++i)
-			pCtx->argv[i] = &aMem[pOp->p2 + i];
-	}
 
 #ifdef SQL_DEBUG
-	for(i = 0; i < argc; i++) {
-		assert(memIsValid(pCtx->argv[i]));
-		REGISTER_TRACE(p, pOp->p2+i, pCtx->argv[i]);
+	for(int i = 0; i < argc; i++) {
+		assert(memIsValid(&aMem[pOp->p2 + i]));
+		REGISTER_TRACE(p, pOp->p2 + i, &aMem[pOp->p2 + i]);
 	}
 #endif
 
 	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);
+	func->call(pCtx, argc, &aMem[pOp->p2]);
 	if (pCtx->is_aborted)
 		goto abort_due_to_error;
 	if (pCtx->skipFlag) {
 		assert(pOp[-1].opcode == OP_SkipLoad);
-		i = pOp[-1].p1;
+		int i = pOp[-1].p1;
 		if (i) mem_set_bool(&aMem[i], true);
 	}
 	break;
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 5e2692d06..8dbba4908 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -179,7 +179,6 @@ struct sql_context {
 	 */
 	bool is_aborted;
 	u8 skipFlag;		/* Skip accumulator loading if true */
-	sql_value *argv[1];	/* Argument set */
 };
 
 /* A bitfield type for use inside of structures.  Always follow with :N where
-- 
2.25.1


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

end of thread, other threads:[~2021-10-01  7:50 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01  7:42 [Tarantool-patches] [PATCH v3 00/15] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 01/15] sql: fix possible undefined behavior during cast Mergen Imeev via Tarantool-patches
2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 02/15] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches
2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 03/15] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 04/15] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches
2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 05/15] sql: introduce mem_append() Mergen Imeev via Tarantool-patches
2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 06/15] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches
2021-10-01  7:42 ` [Tarantool-patches] [PATCH v3 07/15] sql: refactor SUM() function Mergen Imeev via Tarantool-patches
2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 08/15] sql: refactor TOTAL() function Mergen Imeev via Tarantool-patches
2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 09/15] sql: refactor AVG() function Mergen Imeev via Tarantool-patches
2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 10/15] sql: refactor COUNT() function Mergen Imeev via Tarantool-patches
2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 11/15] sql: refactor MIN() and MAX() functions Mergen Imeev via Tarantool-patches
2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 12/15] sql: refactor GROUP_CONCAT() function Mergen Imeev via Tarantool-patches
2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 13/15] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches
2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 14/15] sql: remove MEM_TYPE_AGG Mergen Imeev via Tarantool-patches
2021-10-01  7:43 ` [Tarantool-patches] [PATCH v3 15/15] sql: remove field argv from struct sql_context 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