Re: [Tarantool-patches] [PATCH v5 36/52] sql: introduce mem_set_agg()

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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


On Tue, Apr 13, 2021 at 01:37:49AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index b2598816d..af11ae1d5 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -612,6 +612,23 @@ 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)
> > +{
> > +	if (size <= 0) {
> > +		mem_clear(mem);
> > +		return 0;
> > +	}
> > +	if (sqlVdbeMemGrow(mem, size, 0) != 0)
> > +		return -1;
> > +	memset(mem->z, 0, size);
> > +	mem->n = size;
> > +	mem->flags = MEM_Agg;
> 
> What if it already was MEM_Agg before? sqlVdbeMemGrow()
> didn't clear the old value. It clears only MEM_Dyn.
> 
You are right. Added mem_clear().

> > +	mem->u.func = func;
> > +	mem->field_type = field_type_MAX;
> > +	return 0;
> > +}



Diff:


diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index fdbd15b46..172883a44 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -472,10 +472,9 @@ mem_set_frame(struct Mem *mem, struct VdbeFrame *frame)
 int
 mem_set_agg(struct Mem *mem, struct func *func, int size)
 {
-	if (size <= 0) {
-		mem_clear(mem);
+	mem_clear(mem);
+	if (size <= 0)
 		return 0;
-	}
 	if (sqlVdbeMemGrow(mem, size, 0) != 0)
 		return -1;
 	memset(mem->z, 0, size);



New patch:


commit f8490d6176d02e144e1e3241b04c2c09f8390e78
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Tue Mar 16 15:02:08 2021 +0300

    sql: introduce mem_set_agg()
    
    This patch introduces mem_set_agg() function. This function stores
    aggregation function to MEM and allocates enough memory to hold
    accumulation structure for aggregate function.
    
    Part of #5818

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 8e13131f1..172883a44 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -469,6 +469,22 @@ 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->flags = MEM_Agg;
+	mem->u.func = func;
+	mem->field_type = field_type_MAX;
+	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 36b7d3eca..6dee83dc5 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -546,6 +546,13 @@ mem_set_ptr(struct Mem *mem, void *ptr);
 void
 mem_set_frame(struct Mem *mem, struct VdbeFrame *frame);
 
+/**
+ * Clear the MEM, set the function as its value, and allocate enough memory to
+ * hold the accumulation structure for the aggregate function.
+ */
+int
+mem_set_agg(struct Mem *mem, struct func *func, int size);
+
 /**
  * Copy content of MEM from one MEM to another. In case source MEM contains
  * string or binary and allocation type is not STATIC, this value is copied to
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 65927910a..af1174d0a 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -383,29 +383,6 @@ sqlStmtCurrentTime(sql_context * p)
 	return *piTime;
 }
 
-/*
- * Create a new aggregate context for p and return a pointer to
- * its pMem->z element.
- */
-static SQL_NOINLINE void *
-createAggContext(sql_context * p, int nByte)
-{
-	Mem *pMem = p->pMem;
-	assert(!mem_is_agg(pMem));
-	if (nByte <= 0) {
-		mem_set_null(pMem);
-		pMem->z = 0;
-	} else {
-		sqlVdbeMemClearAndResize(pMem, nByte);
-		pMem->flags = MEM_Agg;
-		pMem->u.func = p->func;
-		if (pMem->z) {
-			memset(pMem->z, 0, nByte);
-		}
-	}
-	return (void *)pMem->z;
-}
-
 /*
  * Allocate or return the aggregate context for a user function.  A new
  * context is allocated on the first call.  Subsequent calls return the
@@ -417,12 +394,9 @@ 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);
-	testcase(nByte < 0);
-	if (!mem_is_agg(p->pMem)) {
-		return createAggContext(p, nByte);
-	} else {
-		return (void *)p->pMem->z;
-	}
+	if (!mem_is_agg(p->pMem) && mem_set_agg(p->pMem, p->func, nByte) != 0)
+		return NULL;
+	return p->pMem->z;
 }
 
 /*