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

[imeevma at tarantool.org]

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


On 30.03.2021 02:06, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 2 comments below.
>
>> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
>> index 078de0e62..0211069c6 100644
>> --- a/src/box/sql/mem.c
>> +++ b/src/box/sql/mem.c
>> @@ -613,6 +613,29 @@ mem_set_frame(struct Mem *mem, struct VdbeFrame *frame)
>>  	mem->field_type = field_type_MAX;
>>  }
>>  
>> +int
>> +mem_prepare_aggregate(struct Mem *mem, struct func *func, int size)
>
> 1. Why is this called 'prepare' when the other setters are called 'set'?
> Maybe use 'set' here as well?
>
Fixed.

>> +{
>> +	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;
>> +	mem->u.func = func;
>> +	mem->field_type = field_type_MAX;
>> +	return 0;
>> +}
>> +
>> +void *
>> +mem_get_aggregate(struct Mem *mem)
>> +{
>> +	return (void *)mem->z;
>
> 2. Void cast should work implicitly here I think. But what is
> more interesting is why do you even need the getter? The other
> mem types don't have getters for the union fields, and it is
> fine I suppose. Would be too much. This one does not even check
> if the type is an aggregate.
I moved this function from here to patch "sql: introduce mem_get_agg()". I check
MEM type there, but almost nothing else changed. This is mostly so because I
actually have no idea how to deal with all aggregate functions used standard
rules for MEM. Maybe it is worth to think about moving aggregate functions,
MEM_Ptr, MEM_Frame, etc from MEM to another internal structure? This way we will
be able to use MEMs only for data.


New patch:

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

    sql: introduce mem_set_agg()
    
    This patch introduces mem_set_agg() function. This functions stores
    given functions 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 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;
+	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 ffab0d616..cf0db62f9 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -364,6 +364,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 6d9103ff2..c2d4b8b8a 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -403,29 +403,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
@@ -437,12 +414,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;
 }
 
 /*