[Tarantool-patches] [PATCH v5 18/52] sql: introduce mem_concat()
Mergen Imeev
imeevma at tarantool.org
Tue Apr 13 19:57:53 MSK 2021
Thank you for the review! My answers, diff and new patch below.
On Sun, Apr 11, 2021 at 08:11:53PM +0200, Vladislav Shpilevoy wrote:
> Good job on the patch!
>
> See 2 comments below.
>
> On 09.04.2021 19:57, Mergen Imeev via Tarantool-patches wrote:
> > This patch introduces mem_concat(). Function mem_concat() concatenates
> > values from two MEMs in case these values are strings or binaries and
> > writes the result to the third MEM.
> >
> > Part of #5818
> > ---
> > src/box/sql/mem.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
> > src/box/sql/mem.h | 8 ++++++
> > src/box/sql/vdbe.c | 50 ++----------------------------------
> > 3 files changed, 74 insertions(+), 48 deletions(-)
> >
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index b417c1007..2d76ef88d 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -326,6 +326,70 @@ mem_move(struct Mem *to, struct Mem *from)
> > return 0;
> > }
> >
> > +static bool
> > +is_result_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
> > + enum field_type type)
>
> 1. Functions called 'is_*' never should change anything.
>
Fixed. Renamed to check_result_null().
> Another question is why do you even need it? It is used in a single place,
> where it could be just inlined. And is not used in a place, where it could
> be applied.
>
I added it here since it was the first commit, which used it. This functions
will be used in all arithmetic and bitwise operations with two operands.
> > +{
> > + mem_clear(result);
> > + result->field_type = type;
> > + return (((a->flags | b->flags) & MEM_Null) != 0);
> > +}
> > +
> > +int
> > +mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
> > +{
> > + assert(result != b);
> > + if (a != result) {
> > + if (is_result_null(a, b, result, FIELD_TYPE_STRING))
> > + return 0;
> > + } else {
> > + if (((a->flags | b->flags) & MEM_Null) != 0) {
> > + mem_clear(a);
> > + result->field_type = FIELD_TYPE_STRING;
> > + return 0;
> > + }
> > + }
> > +
> > + /* Concatenation operation can be applied only to strings and blobs. */
> > + if ((b->flags & (MEM_Str | MEM_Blob)) == 0) {
> > + diag_set(ClientError, ER_INCONSISTENT_TYPES,
> > + "text or varbinary", mem_type_to_str(b));
> > + return -1;
> > + }
> > + if ((a->flags & (MEM_Str | MEM_Blob)) == 0) {
> > + diag_set(ClientError, ER_INCONSISTENT_TYPES,
> > + "text or varbinary", mem_type_to_str(a));
> > + return -1;
> > + }
> > +
> > + /* Moreover, both operands must be of the same type. */
> > + if ((b->flags & MEM_Str) != (a->flags & MEM_Str)) {
> > + diag_set(ClientError, ER_INCONSISTENT_TYPES,
> > + mem_type_to_str(a), mem_type_to_str(b));
> > + return -1;
> > + }
> > +
> > + if (ExpandBlob(a) != 0 || ExpandBlob(b) != 0)
> > + return -1;
> > +
> > + uint32_t size = a->n + b->n;
> > + if ((int)size > sql_get()->aLimit[SQL_LIMIT_LENGTH]) {
> > + diag_set(ClientError, ER_SQL_EXECUTE, "string or blob too big");
> > + return -1;
> > + }
> > + if (sqlVdbeMemGrow(result, size, result == a) != 0)
> > + return -1;
> > +
> > + result->flags = a->flags & (MEM_Str | MEM_Blob);
>
> 2. Why isn't result cleared? What if it was an Agg, or Frame?
> I see before your patch they called vdbe_prepare_null_out(), which
> cleared the mem.
>
In case result != left result is cleared in check_result_null(). In the other
case it is cleared if one of operands is NULL. If it is not cleared than
result == left, which means that if it is not varbinary of string, the error
will be returned. There shouldn't be any problem during destruction, I think.
Also, if we clear result when result == left we will get NULL as result of
concatenation no matter what left and right were. I do not know why result
was cleared previously, since it may lead to result described above.
> > + if ((result->flags & MEM_Blob) != 0)
> > + result->field_type = FIELD_TYPE_VARBINARY;
> > + if (result != a)
> > + memcpy(result->z, a->z, a->n);
> > + memcpy(&result->z[a->n], b->z, b->n);
> > + result->n = size;
> > + return 0;
> > +}
Diff:
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 67c8d90ee..f3bb62369 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -181,8 +181,8 @@ mem_move(struct Mem *to, struct Mem *from)
}
static bool
-is_result_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
- enum field_type type)
+check_result_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
+ enum field_type type)
{
mem_clear(result);
result->field_type = type;
@@ -194,7 +194,7 @@ mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
{
assert(result != b);
if (a != result) {
- if (is_result_null(a, b, result, FIELD_TYPE_STRING))
+ if (check_result_null(a, b, result, FIELD_TYPE_STRING))
return 0;
} else {
if (((a->flags | b->flags) & MEM_Null) != 0) {
New patch:
commit e95f6aea6698865e5c864fa8ae1b5393c4faf553
Author: Mergen Imeev <imeevma at gmail.com>
Date: Sat Mar 13 21:30:54 2021 +0300
sql: introduce mem_concat()
This patch introduces mem_concat(). Function mem_concat() concatenates
values from two MEMs in case these values are strings or binaries and
writes the result to the third MEM.
Part of #5818
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index a7caac014..f3bb62369 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -180,6 +180,70 @@ mem_move(struct Mem *to, struct Mem *from)
from->zMalloc = NULL;
}
+static bool
+check_result_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
+ enum field_type type)
+{
+ mem_clear(result);
+ result->field_type = type;
+ return (((a->flags | b->flags) & MEM_Null) != 0);
+}
+
+int
+mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
+{
+ assert(result != b);
+ if (a != result) {
+ if (check_result_null(a, b, result, FIELD_TYPE_STRING))
+ return 0;
+ } else {
+ if (((a->flags | b->flags) & MEM_Null) != 0) {
+ mem_clear(a);
+ result->field_type = FIELD_TYPE_STRING;
+ return 0;
+ }
+ }
+
+ /* Concatenation operation can be applied only to strings and blobs. */
+ if ((b->flags & (MEM_Str | MEM_Blob)) == 0) {
+ diag_set(ClientError, ER_INCONSISTENT_TYPES,
+ "text or varbinary", mem_type_to_str(b));
+ return -1;
+ }
+ if ((a->flags & (MEM_Str | MEM_Blob)) == 0) {
+ diag_set(ClientError, ER_INCONSISTENT_TYPES,
+ "text or varbinary", mem_type_to_str(a));
+ return -1;
+ }
+
+ /* Moreover, both operands must be of the same type. */
+ if ((b->flags & MEM_Str) != (a->flags & MEM_Str)) {
+ diag_set(ClientError, ER_INCONSISTENT_TYPES,
+ mem_type_to_str(a), mem_type_to_str(b));
+ return -1;
+ }
+
+ if (ExpandBlob(a) != 0 || ExpandBlob(b) != 0)
+ return -1;
+
+ uint32_t size = a->n + b->n;
+ if ((int)size > sql_get()->aLimit[SQL_LIMIT_LENGTH]) {
+ diag_set(ClientError, ER_SQL_EXECUTE, "string or blob too big");
+ return -1;
+ }
+ if (sqlVdbeMemGrow(result, size, result == a) != 0)
+ return -1;
+
+ result->flags = a->flags & (MEM_Str | MEM_Blob);
+ if ((result->flags & MEM_Blob) != 0)
+ result->field_type = FIELD_TYPE_VARBINARY;
+ if (result != a)
+ memcpy(result->z, a->z, a->n);
+ memcpy(&result->z[a->n], b->z, b->n);
+ result->n = size;
+ return 0;
+}
+
static inline bool
mem_has_msgpack_subtype(struct Mem *mem)
{
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 37f6ac5df..d17ed0593 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -322,6 +322,14 @@ mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from);
void
mem_move(struct Mem *to, struct Mem *from);
+/**
+ * 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
+ * result MEM is set to NULL even if the result MEM is actually the first MEM.
+ */
+int
+mem_concat(struct Mem *left, struct Mem *right, struct Mem *result);
+
/**
* Simple type to str convertor. It is used to simplify
* error reporting.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e73ed173d..c017c5091 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1060,57 +1060,11 @@ case OP_ResultRow: {
* types (i.e. TEXT and BLOB).
*/
case OP_Concat: { /* same as TK_CONCAT, in1, in2, out3 */
- i64 nByte;
-
pIn1 = &aMem[pOp->p1];
pIn2 = &aMem[pOp->p2];
- pOut = vdbe_prepare_null_out(p, pOp->p3);
- assert(pIn1!=pOut);
- if (mem_is_any_null(pIn1, pIn2)) {
- /* Force NULL be of type STRING. */
- pOut->field_type = FIELD_TYPE_STRING;
- break;
- }
- /*
- * Concatenation operation can be applied only to
- * strings and blobs.
- */
- if (!mem_is_bytes(pIn1) || !mem_is_bytes(pIn2)) {
- char *inconsistent_type = !mem_is_bytes(pIn1) ?
- mem_type_to_str(pIn1) :
- mem_type_to_str(pIn2);
- diag_set(ClientError, ER_INCONSISTENT_TYPES,
- "text or varbinary", inconsistent_type);
- goto abort_due_to_error;
- }
-
- /* Moreover, both operands must be of the same type. */
- if (!mem_is_same_type(pIn1, pIn2)) {
- diag_set(ClientError, ER_INCONSISTENT_TYPES,
- mem_type_to_str(pIn2), mem_type_to_str(pIn1));
- goto abort_due_to_error;
- }
- if (ExpandBlob(pIn1) != 0 || ExpandBlob(pIn2) != 0)
+ pOut = &aMem[pOp->p3];
+ if (mem_concat(pIn2, pIn1, pOut) != 0)
goto abort_due_to_error;
- nByte = pIn1->n + pIn2->n;
- if (nByte>db->aLimit[SQL_LIMIT_LENGTH]) {
- goto too_big;
- }
- if (sqlVdbeMemGrow(pOut, (int)nByte+2, pOut==pIn2)) {
- goto no_mem;
- }
- if (mem_is_str(pIn1))
- MemSetTypeFlag(pOut, MEM_Str);
- else
- MemSetTypeFlag(pOut, MEM_Blob);
- if (pOut!=pIn2) {
- memcpy(pOut->z, pIn2->z, pIn2->n);
- }
- memcpy(&pOut->z[pIn2->n], pIn1->z, pIn1->n);
- pOut->z[nByte]=0;
- pOut->z[nByte+1] = 0;
- pOut->flags |= MEM_Term;
- pOut->n = (int)nByte;
UPDATE_MAX_BLOBSIZE(pOut);
break;
}
More information about the Tarantool-patches
mailing list