Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2 00/15] sql: reworks aggregate functions
@ 2021-09-21 10:58 Mergen Imeev via Tarantool-patches
  2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 03/15] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 4+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-21 10:58 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

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

Also, this patch makes some refactoring of struct sql_context.

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.

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: rework SUM()
  sql: rework TOTAL()
  sql: rework AVG()
  sql: rework COUNT()
  sql: rework MIN() and MAX()
  sql: rework GROUP_CONCAT()
  sql: remove copying of result in finalizers
  sql: remove MEM_TYPE_AGG
  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                       | 806 ++++++++++-------------
 src/box/sql/main.c                       |   5 +-
 src/box/sql/mem.c                        |  77 +--
 src/box/sql/mem.h                        |  29 +-
 src/box/sql/select.c                     |  29 +-
 src/box/sql/sqlInt.h                     |  23 +-
 src/box/sql/vdbe.c                       | 196 ++----
 src/box/sql/vdbeInt.h                    |   7 +-
 src/box/sql/vdbeapi.c                    |  66 --
 src/box/sql/vdbeaux.c                    |  13 +-
 test/sql-tap/built-in-functions.test.lua |  97 ++-
 13 files changed, 571 insertions(+), 839 deletions(-)

-- 
2.25.1


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

* [Tarantool-patches] [PATCH v2 03/15] sql: remove AggStep0 and OP_BuiltinFunction0
  2021-09-21 10:58 [Tarantool-patches] [PATCH v2 00/15] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
@ 2021-09-21 10:59 ` Mergen Imeev via Tarantool-patches
  2021-09-22 22:46   ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 4+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-21 10:59 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review! My answers, diff and new patch below.

On 15.09.2021 00:21, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> On 10.09.2021 18:01, imeevma@tarantool.org wrote:
>> This patch moves the initialization of sql_context out of the VDBE. This
>> allows us to remove the opcodes OP_BuiltinFunction0 and OP_AggStep0,
>> which work in a rather strange way. Moreover, due to the changes these
>> opcodes make to the VDBEs, it is possible that the estimated size of the
>> VDBE may change during execution of VDBE, which could lead to various
>> problems.
>
> There should no be any problems if VDBE size is estimated right before it
> is copied or whatever the size is used for. Because the updated VDBE would
> return the updated size as well. Correct?
>
This is true. However, there can be a problem in prepared statements. For
example, I once changed P4_FUNC in OP_BuiltinFunction0 to P4_STATIC and one test
started to fail. The problem was exactly as described - before the first
execution, the size of the prepared statement was one, after - another.

I think my explanation is somewhat lacking since I don't remember why I had no
other way to fix this. However, I think the current implementation is easier to
understand and work with.

> I must say, the old behaviour was quite inventive. Allowed to create an
> object at runtime and yet not have ifs on the main opcodes paths. I think
> the reason was to reduce usage of pointers in the opcodes. The goal we used
> to have too.
>
> I am fine with the change though.
>
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index 3b5df5292..6446ef091 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -6744,3 +6749,21 @@ sql_expr_extract_select(struct Parse *parser, struct Select *select)
>>  	parser->parsed_ast.expr =
>>  		sqlExprDup(parser->db, expr_list->a->pExpr, 0);
>>  }
>> +
>> +struct sql_context *
>> +sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc)
>> +{
>> +	uint32_t size = sizeof(struct sql_context);
>> +	if (argc > 1)
>> +		size += (argc - 1) * sizeof(struct Mem);
>
> Why do you allocate + size of mem? I don't see where it is saved
> here or used. ctx->pMem is left not initialized, pOut is zero. So
> where is this size of mem used?
>
This is my mistake. Actually, at some point I thought that the arguments were
given by value. This is not true, but I forgot to change the size of allocated
memory. Fixed.

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


Diff:

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index e27dcb336..459325a88 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -6755,7 +6755,7 @@ 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);
+		size += (argc - 1) * sizeof(struct Mem *);
 	struct sql_context *ctx = sqlDbMallocRawNN(sql_get(), size);
 	if (ctx == NULL)
 		return NULL;


New patch:


commit 8ecde75f62af960261dafcbca41f8aba9b7e3e9e
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Sep 10 14:20:01 2021 +0300

    sql: remove AggStep0 and OP_BuiltinFunction0
    
    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

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

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

* Re: [Tarantool-patches] [PATCH v2 03/15] sql: remove AggStep0 and OP_BuiltinFunction0
  2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 03/15] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
@ 2021-09-22 22:46   ` Vladislav Shpilevoy via Tarantool-patches
  2021-09-25 10:58     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 4+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-09-22 22:46 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hi! Thanks for the fixes!

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

Could you please introduce sql_context_delete() and use it where
appropriate? I am sorry I didn't notice it first time.

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

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

* Re: [Tarantool-patches] [PATCH v2 03/15] sql: remove AggStep0 and OP_BuiltinFunction0
  2021-09-22 22:46   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-09-25 10:58     ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 4+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-25 10:58 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review! My answer, diff and new patch below.

On Thu, Sep 23, 2021 at 12:46:01AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> > diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> > index 92e40aef6..459325a88 100644
> > --- a/src/box/sql/select.c
> > +++ b/src/box/sql/select.c
> > @@ -6744,3 +6749,21 @@ sql_expr_extract_select(struct Parse *parser, struct Select *select)
> >  	parser->parsed_ast.expr =
> >  		sqlExprDup(parser->db, expr_list->a->pExpr, 0);
> >  }
> > +
> > +struct sql_context *
> > +sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc)
> > +{
> 
> Could you please introduce sql_context_delete() and use it where
> appropriate? I am sorry I didn't notice it first time.
> 
Done, thanks.

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


Diff:

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 459325a88..5b57f57a3 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -6767,3 +6767,9 @@ sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc)
 	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 c2701dbde..be31f2b50 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4144,6 +4144,9 @@ 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/vdbeaux.c b/src/box/sql/vdbeaux.c
index b7e44a386..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:


New patch:


commit 8c81a679fea58504e0ffdb67fe2017ba1960949b
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Sep 10 14:20:01 2021 +0300

    sql: remove AggStep0 and OP_BuiltinFunction0
    
    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

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;

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

end of thread, other threads:[~2021-09-25 10:58 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-21 10:58 [Tarantool-patches] [PATCH v2 00/15] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 03/15] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
2021-09-22 22:46   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-25 10:58     ` 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