[Tarantool-patches] [PATCH v1 00/21] Refactor non-standard and non-aggragate functions

[Tarantool-patches] [PATCH v1 00/21] Refactor non-standard and non-aggragate functions

[Mergen Imeev via Tarantool-patches]

This patch-set refactor the built-in SQL functions that were not refactored in
the previous two patch-sets. It also simplifies struct Mem.

https://github.com/tarantool/tarantool/issues/4145
https://github.com/tarantool/tarantool/tree/imeevma/gh-4145-row-sql-builtin-funcs

Mergen Imeev (21):
  sql: refactor CHAR() function
  sql: refactor GREATEST() and LEAST() functions
  sql: refactor HEX() function
  sql: refactor LENGTH() function
  sql: refactor PRINTF() function
  sql: refactor RANDOM() function
  sql: rework RANDOMBLOB() function
  sql: refactor ZEROBLOB() function
  sql: refactor TYPEOF() function
  sql: refactor ROUND() function
  sql: refactor ROW_COUNT() function
  sql: rework UUID() function
  sql: refactor VERSION() function
  sql: refactor UNICODE() function
  sql: refactor of SOUNDEX() function
  sql: refactor REPLACE() function
  sql: refactor QUOTE() function
  sql: remove unused code
  sql: remove MEM_Dyn flag
  sql: remove MEM_Term flag
  sql: make arguments to be const

 src/box/bind.c             |   5 +-
 src/box/sql/func.c         | 937 +++++++++++++++----------------------
 src/box/sql/main.c         |  10 -
 src/box/sql/mem.c          | 197 +-------
 src/box/sql/mem.h          | 160 +------
 src/box/sql/printf.c       |  18 +-
 src/box/sql/sqlInt.h       |  66 +--
 src/box/sql/vdbe.h         |   4 +-
 src/box/sql/vdbeapi.c      | 238 +---------
 src/box/sql/vdbeaux.c      |  31 +-
 src/box/sql/whereexpr.c    |  19 +-
 test/sql-tap/func.test.lua |   4 +-
 test/sql-tap/uuid.test.lua |  11 +-
 13 files changed, 461 insertions(+), 1239 deletions(-)

-- 
2.25.1

[Tarantool-patches] [PATCH v1 01/21] sql: refactor CHAR() function

[Mergen Imeev via Tarantool-patches]

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

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index a3c7d8d20..dd5e7d785 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -738,6 +738,45 @@ func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv)
 		ctx->is_aborted = true;
 }
 
+/** Implementation of the CHAR() function. */
+static void
+func_char(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	if (argc == 0)
+		return mem_set_str_static(ctx->pOut, "", 0);
+	char *str = sqlDbMallocRawNN(sql_get(), argc * 4);
+	if (str == NULL) {
+		ctx->is_aborted = true;
+		return;
+	}
+	uint8_t *ptr = (uint8_t *)str;
+	for (int i = 0; i < argc; ++i) {
+		uint32_t c;
+		if (mem_is_null(&argv[i]))
+			c = 0;
+		else if (!mem_is_uint(&argv[i]) || argv[i].u.u > 0x10ffff)
+			c = 0xfffd;
+		else
+			c = argv[i].u.u;
+		if (c < 0x80) {
+			*ptr++ = c & 0xFF;
+		} else if (c < 0x0800) {
+			*ptr++ = 0xC0 + ((c >> 6) & 0x1F);
+			*ptr++ = 0x80 + (c & 0x3F);
+		} else if (c < 0x10000) {
+			*ptr++ = 0xE0 + ((c >> 12) & 0x0F);
+			*ptr++ = 0x80 + ((c >> 6) & 0x3F);
+			*ptr++ = 0x80 + (c & 0x3F);
+		} else {
+			*ptr++ = 0xF0 + ((c >> 18) & 0x07);
+			*ptr++ = 0x80 + ((c >> 12) & 0x3F);
+			*ptr++ = 0x80 + ((c >> 6) & 0x3F);
+			*ptr++ = 0x80 + (c & 0x3F);
+		}
+	}
+	mem_set_str_allocated(ctx->pOut, str, (char *)ptr - str);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1469,50 +1508,6 @@ unicodeFunc(struct sql_context *context, int argc, struct Mem *argv)
 		sql_result_uint(context, sqlUtf8Read(&z));
 }
 
-/*
- * The char() function takes zero or more arguments, each of which is
- * an integer.  It constructs a string where each character of the string
- * is the unicode character for the corresponding integer argument.
- */
-static void
-charFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	unsigned char *z, *zOut;
-	int i;
-	zOut = z = sql_malloc64(argc * 4 + 1);
-	if (z == NULL) {
-		context->is_aborted = true;
-		return;
-	}
-	for (i = 0; i < argc; i++) {
-		uint64_t x;
-		unsigned c;
-		if (sql_value_type(&argv[i]) == MP_INT)
-			x = 0xfffd;
-		else
-			x = mem_get_uint_unsafe(&argv[i]);
-		if (x > 0x10ffff)
-			x = 0xfffd;
-		c = (unsigned)(x & 0x1fffff);
-		if (c < 0x00080) {
-			*zOut++ = (u8) (c & 0xFF);
-		} else if (c < 0x00800) {
-			*zOut++ = 0xC0 + (u8) ((c >> 6) & 0x1F);
-			*zOut++ = 0x80 + (u8) (c & 0x3F);
-		} else if (c < 0x10000) {
-			*zOut++ = 0xE0 + (u8) ((c >> 12) & 0x0F);
-			*zOut++ = 0x80 + (u8) ((c >> 6) & 0x3F);
-			*zOut++ = 0x80 + (u8) (c & 0x3F);
-		} else {
-			*zOut++ = 0xF0 + (u8) ((c >> 18) & 0x07);
-			*zOut++ = 0x80 + (u8) ((c >> 12) & 0x3F);
-			*zOut++ = 0x80 + (u8) ((c >> 6) & 0x3F);
-			*zOut++ = 0x80 + (u8) (c & 0x3F);
-		}
-	}
-	sql_result_text64(context, (char *)z, zOut - z, sql_free);
-}
-
 /*
  * The hex() function.  Interpret the argument as a blob.  Return
  * a hexadecimal rendering as text.
@@ -1865,7 +1860,7 @@ static struct sql_func_definition definitions[] = {
 	 NULL},
 	{"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", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_STRING, func_char, NULL},
 	{"CHAR_LENGTH", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER,
 	 func_char_length, NULL},
 	{"COALESCE", -1, {FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR, sql_builtin_stub,
-- 
2.25.1

[Tarantool-patches] [PATCH v1 02/21] sql: refactor GREATEST() and LEAST() functions

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c | 102 ++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 52 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index dd5e7d785..7886f5f40 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -777,6 +777,29 @@ func_char(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_str_allocated(ctx->pOut, str, (char *)ptr - str);
 }
 
+/** Implementation of the GREATEST() and LEAST() functions. */
+static void
+func_greatest_least(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	assert(argc > 1);
+	int mask = ctx->func->def->name[0] == 'G' ? -1 : 0;
+	assert(ctx->func->def->name[0] == 'G' ||
+	       ctx->func->def->name[0] == 'L');
+
+	if (mem_is_null(&argv[0]))
+		return;
+	int best = 0;
+	for (int i = 1; i < argc; ++i) {
+		if (mem_is_null(&argv[i]))
+			return;
+		int cmp = mem_cmp_scalar(&argv[best], &argv[i], ctx->coll);
+		if ((cmp ^ mask) >= 0)
+			best = i;
+	}
+	if (mem_copy(ctx->pOut, &argv[best]) != 0)
+		ctx->is_aborted = true;
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -824,37 +847,6 @@ sql_func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_uuid(ctx->pOut, &uuid);
 }
 
-/*
- * Implementation of the non-aggregate min() and max() functions
- */
-static void
-minmaxFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	int i;
-	int iBest;
-	struct coll *pColl;
-	struct func *func = context->func;
-	int mask = sql_func_flag_is_set(func, SQL_FUNC_MAX) ? -1 : 0;
-	if (argc < 2) {
-		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT,
-		mask ? "GREATEST" : "LEAST", "at least two", argc);
-		context->is_aborted = true;
-		return;
-	}
-	pColl = context->coll;
-	assert(mask == -1 || mask == 0);
-	iBest = 0;
-	if (mem_is_null(&argv[0]))
-		return;
-	for (i = 1; i < argc; i++) {
-		if (mem_is_null(&argv[i]))
-			return;
-		if ((mem_cmp_scalar(&argv[iBest], &argv[i], pColl) ^ mask) >= 0)
-			iBest = i;
-	}
-	sql_result_value(context, &argv[iBest]);
-}
-
 /*
  * Return the type of the argument.
  */
@@ -1869,19 +1861,20 @@ static struct sql_func_definition definitions[] = {
 	{"COUNT", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_INTEGER, step_count,
 	 fin_count},
 
-	{"GREATEST", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, minmaxFunc,
-	 NULL},
-	{"GREATEST", -1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, minmaxFunc,
-	 NULL},
-	{"GREATEST", -1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, minmaxFunc,
-	 NULL},
+	{"GREATEST", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER,
+	 func_greatest_least, NULL},
+	{"GREATEST", -1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE,
+	 func_greatest_least, NULL},
+	{"GREATEST", -1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER,
+	 func_greatest_least, NULL},
 	{"GREATEST", -1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY,
-	 minmaxFunc, NULL},
-	{"GREATEST", -1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, minmaxFunc, NULL},
-	{"GREATEST", -1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, minmaxFunc,
-	 NULL},
-	{"GREATEST", -1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, minmaxFunc,
-	 NULL},
+	 func_greatest_least, NULL},
+	{"GREATEST", -1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID,
+	 func_greatest_least, NULL},
+	{"GREATEST", -1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING,
+	 func_greatest_least, NULL},
+	{"GREATEST", -1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR,
+	 func_greatest_least, NULL},
 
 	{"GROUP_CONCAT", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING,
 	 step_group_concat, NULL},
@@ -1896,15 +1889,20 @@ static struct sql_func_definition definitions[] = {
 	{"IFNULL", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR,
 	 sql_builtin_stub, NULL},
 
-	{"LEAST", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, minmaxFunc,
-	 NULL},
-	{"LEAST", -1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, minmaxFunc, NULL},
-	{"LEAST", -1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, minmaxFunc, NULL},
-	{"LEAST", -1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, minmaxFunc,
-	 NULL},
-	{"LEAST", -1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, minmaxFunc, NULL},
-	{"LEAST", -1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, minmaxFunc, NULL},
-	{"LEAST", -1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, minmaxFunc, NULL},
+	{"LEAST", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER,
+	 func_greatest_least, NULL},
+	{"LEAST", -1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE,
+	 func_greatest_least, NULL},
+	{"LEAST", -1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER,
+	 func_greatest_least, NULL},
+	{"LEAST", -1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY,
+	 func_greatest_least, NULL},
+	{"LEAST", -1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID,
+	 func_greatest_least, NULL},
+	{"LEAST", -1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING,
+	 func_greatest_least, NULL},
+	{"LEAST", -1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR,
+	 func_greatest_least, NULL},
 
 	{"LENGTH", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, func_char_length,
 	 NULL},
-- 
2.25.1

[Tarantool-patches] [PATCH v1 03/21] sql: refactor HEX() function

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c | 70 ++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 36 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 7886f5f40..e5d763be1 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -800,6 +800,39 @@ func_greatest_least(struct sql_context *ctx, int argc, struct Mem *argv)
 		ctx->is_aborted = true;
 }
 
+/** Implementation of the HEX() function. */
+static const char hexdigits[] = {
+	'0', '1', '2', '3', '4', '5', '6', '7',
+	'8', '9', 'A', 'B', 'C', 'D', 'E', 'F'
+};
+
+static void
+func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	struct Mem *arg = &argv[0];
+	if (mem_is_null(arg))
+		return mem_set_null(ctx->pOut);
+
+	assert(mem_is_bin(arg) && arg->n >= 0);
+	if (arg->n == 0)
+		return mem_set_str0_static(ctx->pOut, "");
+
+	uint32_t size = 2 * arg->n;
+	char *str = sqlDbMallocRawNN(sql_get(), size);
+	if (str == NULL) {
+		ctx->is_aborted = true;
+		return;
+	}
+	for (int i = 0; i < arg->n; ++i) {
+		char c = arg->z[i];
+		str[2 * i] = hexdigits[(c >> 4) & 0xf];
+		str[2 * i + 1] = hexdigits[c & 0xf];
+	}
+	mem_set_str_allocated(ctx->pOut, str, size);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1383,14 +1416,6 @@ sql_func_version(struct sql_context *context, int argc, struct Mem *argv)
 	sql_result_text(context, tarantool_version(), -1, SQL_STATIC);
 }
 
-/* Array for converting from half-bytes (nybbles) into ASCII hex
- * digits.
- */
-static const char hexdigits[] = {
-	'0', '1', '2', '3', '4', '5', '6', '7',
-	'8', '9', 'A', 'B', 'C', 'D', 'E', 'F'
-};
-
 /*
  * Implementation of the QUOTE() function.  This function takes a single
  * argument.  If the argument is numeric, the return value is the same as
@@ -1500,33 +1525,6 @@ unicodeFunc(struct sql_context *context, int argc, struct Mem *argv)
 		sql_result_uint(context, sqlUtf8Read(&z));
 }
 
-/*
- * The hex() function.  Interpret the argument as a blob.  Return
- * a hexadecimal rendering as text.
- */
-static void
-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 */
-	z = zHex = contextMalloc(context, ((i64) n) * 2 + 1);
-	if (zHex) {
-		for (i = 0; i < n; i++, pBlob++) {
-			unsigned char c = *pBlob;
-			*(z++) = hexdigits[(c >> 4) & 0xf];
-			*(z++) = hexdigits[c & 0xf];
-		}
-		*z = 0;
-		sql_result_text(context, zHex, n * 2, sql_free);
-	}
-}
-
 /*
  * The zeroblob(N) function returns a zero-filled blob of size N bytes.
  */
@@ -1885,7 +1883,7 @@ static struct sql_func_definition definitions[] = {
 	{"GROUP_CONCAT", 2, {FIELD_TYPE_VARBINARY, FIELD_TYPE_VARBINARY},
 	 FIELD_TYPE_VARBINARY, step_group_concat, NULL},
 
-	{"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, hexFunc, NULL},
+	{"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, func_hex, NULL},
 	{"IFNULL", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR,
 	 sql_builtin_stub, NULL},
 
-- 
2.25.1

[Tarantool-patches] [PATCH v1 04/21] sql: refactor LENGTH() function

[Mergen Imeev via Tarantool-patches]

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

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index e5d763be1..863dbf1c4 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -833,6 +833,19 @@ func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_str_allocated(ctx->pOut, str, size);
 }
 
+/** Implementation of the OCTET_LENGTH() function. */
+static void
+func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	struct Mem *arg = &argv[0];
+	if (mem_is_null(arg))
+		return;
+	assert(mem_is_bytes(arg) && arg->n >= 0);
+	mem_set_uint(ctx->pOut, arg->n);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -927,44 +940,6 @@ typeofFunc(struct sql_context *context, int argc, struct Mem *argv)
 	sql_result_text(context, z, -1, SQL_STATIC);
 }
 
-/*
- * Implementation of the length() function
- */
-static void
-lengthFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	int len;
-
-	assert(argc == 1);
-	UNUSED_PARAMETER(argc);
-	switch (sql_value_type(&argv[0])) {
-	case MP_BIN:
-	case MP_ARRAY:
-	case MP_MAP:
-	case MP_INT:
-	case MP_UINT:
-	case MP_BOOL:
-	case MP_DOUBLE:{
-			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]);
-			if (z == 0)
-				return;
-			len = sql_utf8_char_count(z, mem_len_unsafe(&argv[0]));
-			sql_result_uint(context, len);
-			break;
-		}
-	default:{
-			sql_result_null(context);
-			break;
-		}
-	}
-}
-
 /*
  * Implementation of the printf() function.
  */
@@ -1904,8 +1879,8 @@ static struct sql_func_definition definitions[] = {
 
 	{"LENGTH", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, func_char_length,
 	 NULL},
-	{"LENGTH", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_INTEGER, lengthFunc,
-	 NULL},
+	{"LENGTH", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_INTEGER,
+	 func_octet_length, NULL},
 	{"LIKE", 2, {FIELD_TYPE_STRING, FIELD_TYPE_STRING},
 	 FIELD_TYPE_BOOLEAN, likeFunc, NULL},
 	{"LIKE", 3, {FIELD_TYPE_STRING, FIELD_TYPE_STRING, FIELD_TYPE_STRING},
-- 
2.25.1

[Tarantool-patches] [PATCH v1 05/21] sql: refactor PRINTF() function

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c | 71 +++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 863dbf1c4..f5040fb6e 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -846,6 +846,40 @@ func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_uint(ctx->pOut, arg->n);
 }
 
+/** Implementation of the PRINTF() function. */
+static void
+func_printf(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	if (argc < 1 || mem_is_null(&argv[0]))
+		return;
+	if (argc == 1 || !mem_is_str(&argv[0])) {
+		struct Mem *mem = ctx->pOut;
+		if (mem_copy(mem, &argv[0]) != 0 || mem_to_str(mem) != 0)
+			ctx->is_aborted = true;
+		return;
+	}
+	struct PrintfArguments pargs;
+	struct StrAccum acc;
+	char *format = argv[0].z;
+	struct sql *db = sql_get();
+
+	pargs.nArg = argc - 1;
+	pargs.nUsed = 0;
+	pargs.apArg = sqlDbMallocRawNN(db, (argc - 1) * sizeof(*pargs.apArg));
+	if (pargs.apArg == NULL) {
+		ctx->is_aborted = true;
+		return;
+	}
+	for (int i = 1; i < argc; ++i)
+		pargs.apArg[i - 1] = &argv[i];
+	sqlStrAccumInit(&acc, db, 0, 0, db->aLimit[SQL_LIMIT_LENGTH]);
+	acc.printfFlags = SQL_PRINTF_SQLFUNC;
+	sqlXPrintf(&acc, format, &pargs);
+	sqlDbFree(db, pargs.apArg);
+	if (mem_copy_str(ctx->pOut, sqlStrAccumFinish(&acc), acc.nChar) != 0)
+		ctx->is_aborted = true;
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -940,40 +974,6 @@ typeofFunc(struct sql_context *context, int argc, struct Mem *argv)
 	sql_result_text(context, z, -1, SQL_STATIC);
 }
 
-/*
- * Implementation of the printf() function.
- */
-static void
-printfFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	PrintfArguments x;
-	StrAccum str;
-	const char *zFormat;
-	int n;
-	sql *db = sql_context_db_handle(context);
-
-	if (argc >= 1 && (zFormat = mem_as_str0(&argv[0])) != NULL) {
-		x.nArg = argc - 1;
-		x.nUsed = 0;
-		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);
-	}
-}
-
 /*
  * Implementation of the round() function
  */
@@ -1916,8 +1916,7 @@ static struct sql_func_definition definitions[] = {
 	 FIELD_TYPE_INTEGER, func_position_characters, NULL},
 	{"POSITION", 2, {FIELD_TYPE_VARBINARY, FIELD_TYPE_VARBINARY},
 	 FIELD_TYPE_INTEGER, func_position_octets, NULL},
-	{"PRINTF", -1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, printfFunc, 
-	 NULL},
+	{"PRINTF", -1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, func_printf, NULL},
 	{"QUOTE", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, quoteFunc, NULL},
 	{"RANDOM", 0, {}, FIELD_TYPE_INTEGER, randomFunc, NULL},
 	{"RANDOMBLOB", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_VARBINARY,
-- 
2.25.1

[Tarantool-patches] [PATCH v1 06/21] sql: refactor RANDOM() function

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f5040fb6e..8caf389ab 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -880,6 +880,17 @@ func_printf(struct sql_context *ctx, int argc, struct Mem *argv)
 		ctx->is_aborted = true;
 }
 
+/** Implementation of the RANDOM() function. */
+static void
+func_random(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	(void)argc;
+	(void)argv;
+	int64_t r;
+	sql_randomness(sizeof(r), &r);
+	mem_set_int(ctx->pOut, r, r < 0);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1045,29 +1056,6 @@ contextMalloc(struct sql_context *context, i64 nByte)
 	return z;
 }
 
-/*
- * Some functions like COALESCE() and IFNULL() and UNLIKELY() are implemented
- * as VDBE code so that unused argument values do not have to be computed.
- * However, we still need some kind of function implementation for this
- * routines in the function table.  The noopFunc macro provides this.
- * noopFunc will never be called so it doesn't matter what the implementation
- * is.  We might as well use the "version()" function as a substitute.
- */
-#define noopFunc sql_func_version /* Substitute function - never called */
-
-/*
- * Implementation of random().  Return a random integer.
- */
-static void
-randomFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	(void)argc;
-	(void)argv;
-	int64_t r;
-	sql_randomness(sizeof(r), &r);
-	sql_result_int(context, r);
-}
-
 /*
  * Implementation of randomblob(N).  Return a random blob
  * that is N bytes long.
@@ -1918,7 +1906,7 @@ static struct sql_func_definition definitions[] = {
 	 FIELD_TYPE_INTEGER, func_position_octets, NULL},
 	{"PRINTF", -1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, func_printf, NULL},
 	{"QUOTE", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, quoteFunc, NULL},
-	{"RANDOM", 0, {}, FIELD_TYPE_INTEGER, randomFunc, NULL},
+	{"RANDOM", 0, {}, FIELD_TYPE_INTEGER, func_random, NULL},
 	{"RANDOMBLOB", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_VARBINARY,
 	 randomBlob, NULL},
 	{"REPLACE", 3,
-- 
2.25.1

[Tarantool-patches] [PATCH v1 07/21] sql: rework RANDOMBLOB() function

[Mergen Imeev via Tarantool-patches]

This patch refactors RANDOMBLOB() function. Also, RANDOMBLOB(0) now
returns empty string.

part of #4145
---
 src/box/sql/func.c         | 52 +++++++++++++++++---------------------
 test/sql-tap/func.test.lua |  4 +--
 2 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 8caf389ab..51a43f271 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -891,6 +891,28 @@ func_random(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_int(ctx->pOut, r, r < 0);
 }
 
+/** Implementation of the RANDOMBLOB() function. */
+static void
+func_randomblob(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	struct Mem *arg = &argv[0];
+	assert(mem_is_null(arg) || mem_is_int(arg));
+	if (mem_is_null(arg) || !mem_is_uint(arg))
+		return;
+	if (arg->u.u == 0)
+		return mem_set_bin_static(ctx->pOut, "", 0);
+	uint64_t len = arg->u.u;
+	char *res = sqlDbMallocRawNN(sql_get(), len);
+	if (res == NULL) {
+		ctx->is_aborted = true;
+		return;
+	}
+	sql_randomness(len, res);
+	mem_set_bin_allocated(ctx->pOut, res, len);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1056,34 +1078,6 @@ contextMalloc(struct sql_context *context, i64 nByte)
 	return z;
 }
 
-/*
- * Implementation of randomblob(N).  Return a random blob
- * that is N bytes long.
- */
-static void
-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])) {
-		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 mem_str(&argv[0]), "number");
-		context->is_aborted = true;
-		return;
-	}
-	n = mem_get_int_unsafe(&argv[0]);
-	if (n < 1)
-		return;
-	p = contextMalloc(context, n);
-	if (p) {
-		sql_randomness(n, p);
-		sql_result_blob(context, (char *)p, n, sql_free);
-	}
-}
-
 #define Utf8Read(s, e) \
 	ucnv_getNextUChar(icu_utf8_conv, &(s), (e), &status)
 
@@ -1908,7 +1902,7 @@ static struct sql_func_definition definitions[] = {
 	{"QUOTE", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, quoteFunc, NULL},
 	{"RANDOM", 0, {}, FIELD_TYPE_INTEGER, func_random, NULL},
 	{"RANDOMBLOB", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_VARBINARY,
-	 randomBlob, NULL},
+	 func_randomblob, NULL},
 	{"REPLACE", 3,
 	 {FIELD_TYPE_STRING, FIELD_TYPE_STRING, FIELD_TYPE_STRING},
 	 FIELD_TYPE_STRING, replaceFunc, NULL},
diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index dc4dfdc0e..2ce77c6e1 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -2545,8 +2545,8 @@ test:do_execsql_test(
 
 test:do_execsql_test(
     "func-36",
-    [[VALUES (LENGTH(RANDOMBLOB(0)))]],
-    {""})
+    [[VALUES (RANDOMBLOB(0))]],
+    {''})
 
 -- gh-3542
 -- In SQL '\0' is NOT a end-of-string signal. Tests below ensures
-- 
2.25.1

[Tarantool-patches] [PATCH v1 08/21] sql: refactor ZEROBLOB() function

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c | 49 +++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 51a43f271..485ff33c0 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -913,6 +913,27 @@ func_randomblob(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_bin_allocated(ctx->pOut, res, len);
 }
 
+/** Implementation of the ZEROBLOB() function. */
+static void
+func_zeroblob(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	struct Mem *arg = &argv[0];
+	assert(mem_is_null(arg) || mem_is_int(arg));
+	if (mem_is_null(arg) || !mem_is_uint(arg))
+		return;
+	if (arg->u.u == 0)
+		return mem_set_bin_static(ctx->pOut, "", 0);
+	uint64_t len = arg->u.u;
+	char *res = sqlDbMallocZero(sql_get(), len);
+	if (res == NULL) {
+		ctx->is_aborted = true;
+		return;
+	}
+	mem_set_bin_allocated(ctx->pOut, res, len);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1482,32 +1503,6 @@ unicodeFunc(struct sql_context *context, int argc, struct Mem *argv)
 		sql_result_uint(context, sqlUtf8Read(&z));
 }
 
-/*
- * The zeroblob(N) function returns a zero-filled blob of size N bytes.
- */
-static void
-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]);
-	if (n < 0)
-		n = 0;
-	if (n > sql_get()->aLimit[SQL_LIMIT_LENGTH]) {
-		diag_set(ClientError, ER_SQL_EXECUTE, "string or binary string"\
-			 "is too big");
-		context->is_aborted = true;
-		return;
-	}
-	char *str = sqlDbMallocZero(sql_get(), n);
-	if (str == NULL) {
-		context->is_aborted = true;
-		return;
-	}
-	mem_set_bin_allocated(context->pOut, str, n);
-}
-
 /*
  * The replace() function.  Three arguments are all strings: call
  * them A, B, and C. The result is also a string which is derived
@@ -1953,7 +1948,7 @@ static struct sql_func_definition definitions[] = {
 	{"UUID", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_UUID, sql_func_uuid, NULL},
 	{"VERSION", 0, {}, FIELD_TYPE_STRING, sql_func_version, NULL},
 	{"ZEROBLOB", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_VARBINARY,
-	 zeroblobFunc, NULL},
+	 func_zeroblob, NULL},
 };
 
 static struct sql_func_dictionary *
-- 
2.25.1

[Tarantool-patches] [PATCH v1 09/21] sql: refactor TYPEOF() function

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c | 58 ++++++++--------------------------------------
 1 file changed, 10 insertions(+), 48 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 485ff33c0..b1c96ad81 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -934,6 +934,15 @@ func_zeroblob(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_bin_allocated(ctx->pOut, res, len);
 }
 
+/** Implementation of the TYPEOF() function. */
+static void
+func_typeof(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	return mem_set_str0_static(ctx->pOut, mem_type_to_str(&argv[0]));
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -981,53 +990,6 @@ sql_func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_uuid(ctx->pOut, &uuid);
 }
 
-/*
- * Return the type of the argument.
- */
-static void
-typeofFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	(void)argc;
-	const char *z = 0;
-	if ((argv[0].flags & MEM_Number) != 0)
-		return mem_set_str0_static(context->pOut, "number");
-	if ((argv[0].flags & MEM_Scalar) != 0)
-		return mem_set_str0_static(context->pOut, "scalar");
-	switch (argv[0].type) {
-	case MEM_TYPE_INT:
-	case MEM_TYPE_UINT:
-		z = "integer";
-		break;
-	case MEM_TYPE_DEC:
-		z = "decimal";
-		break;
-	case MEM_TYPE_STR:
-		z = "string";
-		break;
-	case MEM_TYPE_DOUBLE:
-		z = "double";
-		break;
-	case MEM_TYPE_BIN:
-	case MEM_TYPE_ARRAY:
-	case MEM_TYPE_MAP:
-		z = "varbinary";
-		break;
-	case MEM_TYPE_BOOL:
-		z = "boolean";
-		break;
-	case MEM_TYPE_NULL:
-		z = "NULL";
-		break;
-	case MEM_TYPE_UUID:
-		z = "uuid";
-		break;
-	default:
-		unreachable();
-		break;
-	}
-	sql_result_text(context, z, -1, SQL_STATIC);
-}
-
 /*
  * Implementation of the round() function
  */
@@ -1937,7 +1899,7 @@ static struct sql_func_definition definitions[] = {
 	 {FIELD_TYPE_VARBINARY, FIELD_TYPE_INTEGER, FIELD_TYPE_VARBINARY},
 	 FIELD_TYPE_VARBINARY, func_trim_bin, NULL},
 
-	{"TYPEOF", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, typeofFunc, NULL},
+	{"TYPEOF", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, func_typeof, NULL},
 	{"UNICODE", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, unicodeFunc,
 	 NULL},
 	{"UNLIKELY", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_BOOLEAN, sql_builtin_stub,
-- 
2.25.1

[Tarantool-patches] [PATCH v1 10/21] sql: refactor ROUND() function

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c | 75 +++++++++++++++++-----------------------------
 1 file changed, 28 insertions(+), 47 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index b1c96ad81..65da47da1 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -943,6 +943,32 @@ func_typeof(struct sql_context *ctx, int argc, struct Mem *argv)
 	return mem_set_str0_static(ctx->pOut, mem_type_to_str(&argv[0]));
 }
 
+/** Implementation of the ROUND() function. */
+static void
+func_round(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	assert(argc == 1 || argc == 2);
+	if (mem_is_null(&argv[0]) || (argc == 2 && mem_is_null(&argv[1])))
+		return;
+	assert(mem_is_double(&argv[0]));
+	assert(argc == 1 || mem_is_int(&argv[1]));
+	uint64_t n = (argc == 2 && mem_is_uint(&argv[1])) ? argv[1].u.u : 0;
+
+	double d = argv[0].u.r;
+	struct Mem *res = ctx->pOut;
+	if (n != 0)
+		return mem_set_double(res, atof(tt_sprintf("%.*f", n, d)));
+	/*
+	 * DOUBLE values greater than 2^53 or less than -2^53 have no digits
+	 * after the decimal point.
+	 */
+	assert(9007199254740992 == (int64_t)1 << 53);
+	if (d <= -9007199254740992.0 || d >= 9007199254740992.0)
+		return mem_set_double(res, d);
+	double delta = d < 0 ? -0.5 : 0.5;
+	return mem_set_double(res, (double)(int64_t)(d + delta));
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -990,51 +1016,6 @@ sql_func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_uuid(ctx->pOut, &uuid);
 }
 
-/*
- * Implementation of the round() function
- */
-static void
-roundFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	int64_t n = 0;
-	double r;
-	if (argc != 1 && argc != 2) {
-		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "ROUND",
-			 "1 or 2", argc);
-		context->is_aborted = true;
-		return;
-	}
-	if (argc == 2) {
-		if (mem_is_null(&argv[1]))
-			return;
-		n = mem_get_int_unsafe(&argv[1]);
-		if (n < 0)
-			n = 0;
-	}
-	if (mem_is_null(&argv[0]))
-		return;
-	if (!mem_is_num(&argv[0]) && !mem_is_str(&argv[0])) {
-		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 mem_str(&argv[0]), "number");
-		context->is_aborted = true;
-		return;
-	}
-	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.
-	 */
-	if (n == 0 && r >= 0 && r < (double)(LARGEST_INT64 - 1)) {
-		r = (double)((sql_int64) (r + 0.5));
-	} else if (n == 0 && r < 0 && (-r) < (double)(LARGEST_INT64 - 1)) {
-		r = -(double)((sql_int64) ((-r) + 0.5));
-	} else {
-		const char *rounded_value = tt_sprintf("%.*f", n, r);
-		sqlAtoF(rounded_value, &r, sqlStrlen30(rounded_value));
-	}
-	sql_result_double(context, r);
-}
-
 /*
  * Allocate nByte bytes of space using sqlMalloc(). If the
  * allocation fails, return NULL. If nByte is larger than the
@@ -1866,9 +1847,9 @@ static struct sql_func_definition definitions[] = {
 	{"REPLACE", 3,
 	 {FIELD_TYPE_VARBINARY, FIELD_TYPE_VARBINARY, FIELD_TYPE_VARBINARY},
 	 FIELD_TYPE_VARBINARY, replaceFunc, NULL},
-	{"ROUND", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, roundFunc, NULL},
+	{"ROUND", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, func_round, NULL},
 	{"ROUND", 2, {FIELD_TYPE_DOUBLE, FIELD_TYPE_INTEGER}, FIELD_TYPE_DOUBLE,
-	 roundFunc, NULL},
+	 func_round, NULL},
 	{"ROW_COUNT", 0, {}, FIELD_TYPE_INTEGER, sql_row_count, NULL},
 	{"SOUNDEX", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, soundexFunc,
 	 NULL},
-- 
2.25.1

[Tarantool-patches] [PATCH v1 11/21] sql: refactor ROW_COUNT() function

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c   | 12 +++++++++++-
 src/box/sql/main.c   | 10 ----------
 src/box/sql/sqlInt.h |  8 +-------
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 65da47da1..ca7c41d96 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -969,6 +969,16 @@ func_round(struct sql_context *ctx, int argc, struct Mem *argv)
 	return mem_set_double(res, (double)(int64_t)(d + delta));
 }
 
+/** Implementation of the ROW_COUNT() function. */
+static void
+func_row_count(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	(void)argc;
+	(void)argv;
+	assert(sql_get()->nChange >= 0);
+	return mem_set_uint(ctx->pOut, sql_get()->nChange);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1850,7 +1860,7 @@ static struct sql_func_definition definitions[] = {
 	{"ROUND", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, func_round, NULL},
 	{"ROUND", 2, {FIELD_TYPE_DOUBLE, FIELD_TYPE_INTEGER}, FIELD_TYPE_DOUBLE,
 	 func_round, NULL},
-	{"ROW_COUNT", 0, {}, FIELD_TYPE_INTEGER, sql_row_count, NULL},
+	{"ROW_COUNT", 0, {}, FIELD_TYPE_INTEGER, func_row_count, NULL},
 	{"SOUNDEX", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, soundexFunc,
 	 NULL},
 	{"SUBSTR", 2, {FIELD_TYPE_STRING, FIELD_TYPE_INTEGER},
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index a4247c760..ae872aaa1 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -220,16 +220,6 @@ setupLookaside(sql * db, void *pBuf, int sz, int cnt)
 	return 0;
 }
 
-void
-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);
-}
-
 /*
  * Close all open savepoints.
  * This procedure is trivial as savepoints are allocated on the "region" and
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index cfdf71f1f..72f349944 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -470,12 +470,6 @@ enum sql_subtype {
 void
 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, int argc, struct Mem *argv);
-
 int
 sql_column_count(sql_stmt * pStmt);
 
@@ -1065,7 +1059,7 @@ struct sql {
 	u8 dfltLockMode;	/* Default locking-mode for attached dbs */
 	u8 mTrace;		/* zero or more sql_TRACE flags */
 	u32 magic;		/* Magic number for detect library misuse */
-	/** Value returned by sql_row_count(). */
+	/** Value returned by ROW_COUNT(). */
 	int nChange;
 	int aLimit[SQL_N_LIMIT];	/* Limits */
 	int nMaxSorterMmap;	/* Maximum size of regions mapped by sorter */
-- 
2.25.1

[Tarantool-patches] [PATCH v1 12/21] sql: rework UUID() function

[Mergen Imeev via Tarantool-patches]

This patch refactors UUID() function. Also, UUID(NULL) now returns NULL.

Part of #4145
---
 src/box/sql/func.c         | 52 +++++++++++++++-----------------------
 test/sql-tap/uuid.test.lua | 11 +++++++-
 2 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index ca7c41d96..d462f66b5 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -979,6 +979,25 @@ func_row_count(struct sql_context *ctx, int argc, struct Mem *argv)
 	return mem_set_uint(ctx->pOut, sql_get()->nChange);
 }
 
+/** Implementation of the UUID() function. */
+static void
+func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	if (argc == 1) {
+		if (mem_is_null(&argv[0]))
+			return;
+		if (!mem_is_uint(&argv[0]) || argv[0].u.u != 4) {
+			diag_set(ClientError, ER_UNSUPPORTED, "Function UUID",
+				 "versions other than 4");
+			ctx->is_aborted = true;
+			return;
+		}
+	}
+	struct tt_uuid uuid;
+	tt_uuid_create(&uuid);
+	mem_set_uuid(ctx->pOut, &uuid);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -997,35 +1016,6 @@ mem_as_bin(struct Mem *mem)
 	return s;
 }
 
-static void
-sql_func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
-{
-	if (argc > 1) {
-		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "UUID",
-			 "one or zero", argc);
-		ctx->is_aborted = true;
-		return;
-	}
-	if (argc == 1) {
-		uint64_t version;
-		if (mem_get_uint(&argv[0], &version) != 0) {
-			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-				 mem_str(&argv[0]), "integer");
-			ctx->is_aborted = true;
-			return;
-		}
-		if (version != 4) {
-			diag_set(ClientError, ER_UNSUPPORTED, "Function UUID",
-				 "versions other than 4");
-			ctx->is_aborted = true;
-			return;
-		}
-	}
-	struct tt_uuid uuid;
-	tt_uuid_create(&uuid);
-	mem_set_uuid(ctx->pOut, &uuid);
-}
-
 /*
  * Allocate nByte bytes of space using sqlMalloc(). If the
  * allocation fails, return NULL. If nByte is larger than the
@@ -1897,8 +1887,8 @@ static struct sql_func_definition definitions[] = {
 	 NULL},
 	{"UPPER", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, func_lower_upper,
 	 NULL},
-	{"UUID", 0, {}, FIELD_TYPE_UUID, sql_func_uuid, NULL},
-	{"UUID", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_UUID, sql_func_uuid, NULL},
+	{"UUID", 0, {}, FIELD_TYPE_UUID, func_uuid, NULL},
+	{"UUID", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_UUID, func_uuid, NULL},
 	{"VERSION", 0, {}, FIELD_TYPE_STRING, sql_func_version, NULL},
 	{"ZEROBLOB", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_VARBINARY,
 	 func_zeroblob, NULL},
diff --git a/test/sql-tap/uuid.test.lua b/test/sql-tap/uuid.test.lua
index fea9d9370..3eb8b4578 100755
--- a/test/sql-tap/uuid.test.lua
+++ b/test/sql-tap/uuid.test.lua
@@ -3,7 +3,7 @@ local build_path = os.getenv("BUILDDIR")
 package.cpath = build_path..'/test/sql-tap/?.so;'..build_path..'/test/sql-tap/?.dylib;'..package.cpath
 
 local test = require("sqltester")
-test:plan(146)
+test:plan(147)
 
 local uuid = require("uuid")
 local uuid1 = uuid.fromstr("11111111-1111-1111-1111-111111111111")
@@ -1290,6 +1290,15 @@ test:do_execsql_test(
         true
     })
 
+-- Make sure the uuid(NULL) returns NULL.
+test:do_execsql_test(
+    "uuid-16.7",
+    [[
+        SELECT uuid(NULL) IS NULL;
+    ]], {
+        true
+    })
+
 -- Make sure STRING of wrong length cannot be cast to UUID.
 test:do_catchsql_test(
     "uuid-17.1",
-- 
2.25.1

[Tarantool-patches] [PATCH v1 13/21] sql: refactor VERSION() function

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index d462f66b5..fb7fd772e 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -998,6 +998,15 @@ func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_uuid(ctx->pOut, &uuid);
 }
 
+/** Implementation of the VERSION() function. */
+static void
+func_version(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	(void)argc;
+	(void)argv;
+	return mem_set_str0_static(ctx->pOut, (char *)tarantool_version());
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1321,22 +1330,6 @@ likeFunc(sql_context *context, int argc, struct Mem *argv)
 	mem_set_bool(context->pOut, res == MATCH);
 }
 
-/**
- * Implementation of the version() function.  The result is the
- * version of the Tarantool that is running.
- *
- * @param context Context being used.
- * @param unused1 Unused.
- * @param unused2 Unused.
- */
-static void
-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);
-}
-
 /*
  * Implementation of the QUOTE() function.  This function takes a single
  * argument.  If the argument is numeric, the return value is the same as
@@ -1889,7 +1882,7 @@ static struct sql_func_definition definitions[] = {
 	 NULL},
 	{"UUID", 0, {}, FIELD_TYPE_UUID, func_uuid, NULL},
 	{"UUID", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_UUID, func_uuid, NULL},
-	{"VERSION", 0, {}, FIELD_TYPE_STRING, sql_func_version, NULL},
+	{"VERSION", 0, {}, FIELD_TYPE_STRING, func_version, NULL},
 	{"ZEROBLOB", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_VARBINARY,
 	 func_zeroblob, NULL},
 };
-- 
2.25.1

[Tarantool-patches] [PATCH v1 14/21] sql: refactor UNICODE() function

[Mergen Imeev via Tarantool-patches]

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

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index fb7fd772e..5e12ef729 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1007,6 +1007,19 @@ func_version(struct sql_context *ctx, int argc, struct Mem *argv)
 	return mem_set_str0_static(ctx->pOut, (char *)tarantool_version());
 }
 
+/** Implementation of the UNICODE() function. */
+static void
+func_unicode(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	if (mem_is_null(&argv[0]))
+		return;
+	assert(mem_is_str(&argv[0]));
+	const char *str = tt_cstr(argv[0].z, argv[0].n);
+	mem_set_uint(ctx->pOut, sqlUtf8Read((const unsigned char **)&str));
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1426,19 +1439,6 @@ quoteFunc(struct sql_context *context, int argc, struct Mem *argv)
 	}
 }
 
-/*
- * The unicode() function.  Return the integer unicode code-point value
- * for the first character of the input string.
- */
-static void
-unicodeFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	const unsigned char *z = mem_as_ustr(&argv[0]);
-	(void)argc;
-	if (z && z[0])
-		sql_result_uint(context, sqlUtf8Read(&z));
-}
-
 /*
  * The replace() function.  Three arguments are all strings: call
  * them A, B, and C. The result is also a string which is derived
@@ -1874,7 +1874,7 @@ static struct sql_func_definition definitions[] = {
 	 FIELD_TYPE_VARBINARY, func_trim_bin, NULL},
 
 	{"TYPEOF", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, func_typeof, NULL},
-	{"UNICODE", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, unicodeFunc,
+	{"UNICODE", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, func_unicode,
 	 NULL},
 	{"UNLIKELY", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_BOOLEAN, sql_builtin_stub,
 	 NULL},
-- 
2.25.1

[Tarantool-patches] [PATCH v1 15/21] sql: refactor of SOUNDEX() function

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 5e12ef729..a3ca53545 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1555,16 +1555,11 @@ soundexFunc(struct sql_context *context, int argc, struct Mem *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])) {
-		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 mem_str(&argv[0]), "string");
-		context->is_aborted = true;
-		return;
-	}
-	zIn = (u8 *) mem_as_ustr(&argv[0]);
-	if (zIn == 0)
+	assert(mem_is_null(&argv[0]) || mem_is_str(&argv[0]));
+	if (mem_is_null(&argv[0]) || argv[0].n == 0)
 		zIn = (u8 *) "";
+	else
+		zIn = (unsigned char *)argv[0].z;
 	for (i = 0; zIn[i] && !sqlIsalpha(zIn[i]); i++) {
 	}
 	if (zIn[i]) {
@@ -1585,12 +1580,10 @@ soundexFunc(struct sql_context *context, int argc, struct Mem *argv)
 			zResult[j++] = '0';
 		}
 		zResult[j] = 0;
-		sql_result_text(context, zResult, 4, SQL_TRANSIENT);
+		if (mem_copy_str(context->pOut, zResult, 4) != 0)
+			context->is_aborted = true;
 	} else {
-		/* IMP: R-64894-50321 The string "?000" is returned if the argument
-		 * is NULL or contains no ASCII alphabetic characters.
-		 */
-		sql_result_text(context, "?000", 4, SQL_STATIC);
+		mem_set_str_static(context->pOut, "?000", 4);
 	}
 }
 
-- 
2.25.1

[Tarantool-patches] [PATCH v1 16/21] sql: refactor REPLACE() function

[Mergen Imeev via Tarantool-patches]

Part of #4145
---
 src/box/sql/func.c | 58 ++++++++++++++++------------------------------
 1 file changed, 20 insertions(+), 38 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index a3ca53545..f26d101e9 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1460,34 +1460,26 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
 	int i, j;		/* Loop counters */
 
 	assert(argc == 3);
-	UNUSED_PARAMETER(argc);
-	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]);
-	if (zPattern == 0) {
-		assert(mem_is_null(&argv[1])
-		       || sql_context_db_handle(context)->mallocFailed);
+	(void)argc;
+	if (mem_is_any_null(&argv[0], &argv[1]) || mem_is_null(&argv[2]))
 		return;
-	}
-	nPattern = mem_len_unsafe(&argv[1]);
+	assert(mem_is_bytes);
+	zStr = (const unsigned char *)argv[0].z;
+	nStr = argv[0].n;
+	zPattern = (const unsigned char *)argv[1].z;
+	nPattern = argv[1].n;
 	if (nPattern == 0) {
-		assert(!mem_is_null(&argv[1]));
-		sql_result_value(context, &argv[0]);
+		if (mem_copy(context->pOut, &argv[0]) != 0)
+			context->is_aborted = true;
 		return;
 	}
-	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]));
+	zRep = (const unsigned char *)argv[2].z;
+	nRep = argv[2].n;
 	nOut = nStr + 1;
-	assert(nOut < SQL_MAX_LENGTH);
-	zOut = contextMalloc(context, (i64) nOut);
-	if (zOut == 0) {
+	struct sql *db = sql_get();
+	zOut = sqlDbMallocRawNN(db, nOut);
+	if (zOut == NULL) {
+		context->is_aborted = true;
 		return;
 	}
 	loopLimit = nStr - nPattern;
@@ -1497,22 +1489,12 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
 			zOut[j++] = zStr[i];
 		} else {
 			u8 *zOld;
-			sql *db = sql_context_db_handle(context);
 			nOut += nRep - nPattern;
-			testcase(nOut - 1 == db->aLimit[SQL_LIMIT_LENGTH]);
-			testcase(nOut - 2 == db->aLimit[SQL_LIMIT_LENGTH]);
-			if (nOut - 1 > db->aLimit[SQL_LIMIT_LENGTH]) {
-				diag_set(ClientError, ER_SQL_EXECUTE, "string "\
-					 "or binary string is too big");
-				context->is_aborted = true;
-				sql_free(zOut);
-				return;
-			}
 			zOld = zOut;
-			zOut = sql_realloc64(zOut, (int)nOut);
-			if (zOut == 0) {
+			zOut = sqlDbRealloc(db, zOut, nOut);
+			if (zOut == NULL) {
 				context->is_aborted = true;
-				sql_free(zOld);
+				sqlDbFree(db, zOld);
 				return;
 			}
 			memcpy(&zOut[j], zRep, nRep);
@@ -1526,9 +1508,9 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
 	assert(j <= nOut);
 	zOut[j] = 0;
 	if (context->func->def->returns == FIELD_TYPE_STRING)
-		mem_set_str_dynamic(context->pOut, (char *)zOut, j);
+		mem_set_str_allocated(context->pOut, (char *)zOut, j);
 	else
-		mem_set_bin_dynamic(context->pOut, (char *)zOut, j);
+		mem_set_bin_allocated(context->pOut, (char *)zOut, j);
 }
 
 /*
-- 
2.25.1

[Tarantool-patches] [PATCH v1 17/21] sql: refactor QUOTE() function

[Mergen Imeev via Tarantool-patches]

---
 src/box/sql/func.c | 154 +++++++++++++++------------------------------
 1 file changed, 50 insertions(+), 104 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f26d101e9..4e7404337 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1020,50 +1020,6 @@ func_unicode(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_uint(ctx->pOut, sqlUtf8Read((const unsigned char **)&str));
 }
 
-static const unsigned char *
-mem_as_ustr(struct Mem *mem)
-{
-	return (const unsigned char *)mem_as_str0(mem);
-}
-
-static const void *
-mem_as_bin(struct Mem *mem)
-{
-	const char *s;
-	if (mem_cast_explicit(mem, FIELD_TYPE_VARBINARY) != 0 &&
-	    mem_to_str(mem) != 0)
-		return NULL;
-	if (mem_get_bin(mem, &s) != 0)
-		return NULL;
-	return s;
-}
-
-/*
- * Allocate nByte bytes of space using sqlMalloc(). If the
- * allocation fails, return NULL. If nByte is larger than the
- * maximum string or blob length, then raise an error and return
- * NULL.
- */
-static void *
-contextMalloc(struct sql_context *context, i64 nByte)
-{
-	char *z;
-	sql *db = sql_context_db_handle(context);
-	assert(nByte > 0);
-	testcase(nByte == db->aLimit[SQL_LIMIT_LENGTH]);
-	testcase(nByte == db->aLimit[SQL_LIMIT_LENGTH] + 1);
-	if (nByte > db->aLimit[SQL_LIMIT_LENGTH]) {
-		diag_set(ClientError, ER_SQL_EXECUTE, "string or blob too big");
-		context->is_aborted = true;
-		z = 0;
-	} else {
-		z = sqlMalloc(nByte);
-		if (z == NULL)
-			context->is_aborted = true;
-	}
-	return z;
-}
-
 #define Utf8Read(s, e) \
 	ucnv_getNextUChar(icu_utf8_conv, &(s), (e), &status)
 
@@ -1359,83 +1315,73 @@ quoteFunc(struct sql_context *context, int argc, struct Mem *argv)
 	case MEM_TYPE_UUID: {
 		char buf[UUID_STR_LEN + 1];
 		tt_uuid_to_string(&argv[0].u.uuid, &buf[0]);
-		sql_result_text(context, buf, UUID_STR_LEN, SQL_TRANSIENT);
+		if (mem_copy_str(context->pOut, buf, UUID_STR_LEN) != 0)
+			context->is_aborted = true;
 		break;
 	}
 	case MEM_TYPE_DOUBLE:
 	case MEM_TYPE_DEC:
 	case MEM_TYPE_UINT:
 	case MEM_TYPE_INT: {
-			sql_result_value(context, &argv[0]);
-			break;
-		}
+		if (mem_copy(context->pOut, &argv[0]) != 0)
+			context->is_aborted = true;
+		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 */
-			zText =
-			    (char *)contextMalloc(context,
-						  (2 * (i64) nBlob) + 4);
-			if (zText) {
-				int i;
-				for (i = 0; i < nBlob; i++) {
-					zText[(i * 2) + 2] =
-					    hexdigits[(zBlob[i] >> 4) & 0x0F];
-					zText[(i * 2) + 3] =
-					    hexdigits[(zBlob[i]) & 0x0F];
-				}
-				zText[(nBlob * 2) + 2] = '\'';
-				zText[(nBlob * 2) + 3] = '\0';
-				zText[0] = 'X';
-				zText[1] = '\'';
-				sql_result_text(context, zText, -1,
-						    SQL_TRANSIENT);
-				sql_free(zText);
-			}
-			break;
+		const char *zBlob = argv[0].z;
+		int nBlob = argv[0].n;
+		uint32_t size = 2 * nBlob + 3;
+		char *zText = zText = sqlDbMallocRawNN(sql_get(), size);
+		if (zText == NULL) {
+			context->is_aborted = true;
+			return;
 		}
+		for (int i = 0; i < nBlob; i++) {
+			zText[(i * 2) + 2] = hexdigits[(zBlob[i] >> 4) & 0x0F];
+			zText[(i * 2) + 3] = hexdigits[(zBlob[i]) & 0x0F];
+		}
+		zText[(nBlob * 2) + 2] = '\'';
+		zText[0] = 'X';
+		zText[1] = '\'';
+		mem_set_str_allocated(context->pOut, zText, size);
+		break;
+	}
 	case MEM_TYPE_STR: {
-			int i, j;
-			u64 n;
-			const unsigned char *zArg = mem_as_ustr(&argv[0]);
-			char *z;
+		const char *str = argv[0].z;
+		uint32_t len = argv[0].n;
+		uint32_t count = 0;
+		for (uint32_t i = 0; i < len; ++i) {
+			if (str[i] == '\'')
+				++count;
+		}
+		uint32_t size = len + count + 2;
 
-			if (zArg == 0)
-				return;
-			for (i = 0, n = 0; zArg[i]; i++) {
-				if (zArg[i] == '\'')
-					n++;
-			}
-			z = contextMalloc(context, ((i64) i) + ((i64) n) + 3);
-			if (z) {
-				z[0] = '\'';
-				for (i = 0, j = 1; zArg[i]; i++) {
-					z[j++] = zArg[i];
-					if (zArg[i] == '\'') {
-						z[j++] = '\'';
-					}
-				}
-				z[j++] = '\'';
-				z[j] = 0;
-				sql_result_text(context, z, j,
-						    sql_free);
-			}
-			break;
+		char *res = sqlDbMallocRawNN(sql_get(), size);
+		if (res == NULL) {
+			context->is_aborted = true;
+			return;
+		}
+		res[0] = '\'';
+		for (uint32_t i = 0, j = 1; i < len; ++i) {
+			res[j++] = str[i];
+			if (str[i] == '\'')
+				res[j++] = '\'';
 		}
+		res[size - 1] = '\'';
+		mem_set_str_allocated(context->pOut, res, size);
+		break;
+	}
 	case MEM_TYPE_BOOL: {
-		sql_result_text(context,
-				SQL_TOKEN_BOOLEAN(argv[0].u.b),
-				-1, SQL_TRANSIENT);
+		mem_set_str0_static(context->pOut,
+				    SQL_TOKEN_BOOLEAN(argv[0].u.b));
 		break;
 	}
 	default:{
-			assert(mem_is_null(&argv[0]));
-			sql_result_text(context, "NULL", 4, SQL_STATIC);
-			break;
-		}
+		assert(mem_is_null(&argv[0]));
+		mem_set_str0_static(context->pOut, "NULL");
+	}
 	}
 }
 
-- 
2.25.1

[Tarantool-patches] [PATCH v1 18/21] sql: remove unused code

[Mergen Imeev via Tarantool-patches]

Some of the code is no longer used after changes in the SQL built-in
functions. This patch removes part of the unused code.

Needed for #4145
---
 src/box/bind.c        |   5 +-
 src/box/sql/mem.c     |  11 --
 src/box/sql/mem.h     |  71 -------------
 src/box/sql/sqlInt.h  |  53 +---------
 src/box/sql/vdbeapi.c | 238 +++---------------------------------------
 5 files changed, 19 insertions(+), 359 deletions(-)

diff --git a/src/box/bind.c b/src/box/bind.c
index 58fff0b98..e75e36283 100644
--- a/src/box/bind.c
+++ b/src/box/bind.c
@@ -185,12 +185,11 @@ sql_bind_column(struct sql_stmt *stmt, const struct sql_bind *p,
 		 * there is no need to copy the packet and we can
 		 * use SQL_STATIC.
 		 */
-		return sql_bind_text64(stmt, pos, p->s, p->bytes, SQL_STATIC);
+		return sql_bind_str_static(stmt, pos, p->s, p->bytes);
 	case MP_NIL:
 		return sql_bind_null(stmt, pos);
 	case MP_BIN:
-		return sql_bind_blob64(stmt, pos, (const void *) p->s, p->bytes,
-				       SQL_STATIC);
+		return sql_bind_bin_static(stmt, pos, p->s, p->bytes);
 	case MP_EXT:
 		assert(p->ext_type == MP_UUID || p->ext_type == MP_DECIMAL);
 		if (p->ext_type == MP_UUID)
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index fe32a7ad7..895003580 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2633,17 +2633,6 @@ mem_mp_type(const struct Mem *mem)
 	return MP_NIL;
 }
 
-/* EVIDENCE-OF: R-12793-43283 Every value in sql has one of five
- * fundamental datatypes: 64-bit signed integer 64-bit IEEE floating
- * point number string BLOB NULL
- */
-enum mp_type
-sql_value_type(sql_value *pVal)
-{
-	struct Mem *mem = (struct Mem *) pVal;
-	return mem_mp_type(mem);
-}
-
 #ifdef SQL_DEBUG
 /*
  * Check invariants on a Mem object.
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 7f7382961..1d995726f 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -351,54 +351,6 @@ mem_set_str0_dynamic(struct Mem *mem, char *value);
 void
 mem_set_str0_allocated(struct Mem *mem, char *value);
 
-static inline void
-mem_set_strl_ephemeral(struct Mem *mem, char *value, int len_hint)
-{
-	if (len_hint < 0)
-		mem_set_str0_ephemeral(mem, value);
-	else
-		mem_set_str_ephemeral(mem, value, len_hint);
-}
-
-static inline void
-mem_set_strl_static(struct Mem *mem, char *value, int len_hint)
-{
-	if (len_hint < 0)
-		mem_set_str0_static(mem, value);
-	else
-		mem_set_str_static(mem, value, len_hint);
-}
-
-static inline void
-mem_set_strl_dynamic(struct Mem *mem, char *value, int len_hint)
-{
-	if (len_hint < 0)
-		mem_set_str0_dynamic(mem, value);
-	else
-		mem_set_str_dynamic(mem, value, len_hint);
-}
-
-static inline void
-mem_set_strl_allocated(struct Mem *mem, char *value, int len_hint)
-{
-	if (len_hint < 0)
-		mem_set_str0_allocated(mem, value);
-	else
-		mem_set_str_allocated(mem, value, len_hint);
-}
-
-static inline void
-mem_set_strl(struct Mem *mem, char *value, int len_hint,
-	     void (*custom_free)(void *))
-{
-	if (custom_free == SQL_STATIC)
-		return mem_set_strl_static(mem, value, len_hint);
-	if (custom_free == SQL_DYNAMIC)
-		return mem_set_strl_allocated(mem, value, len_hint);
-	if (custom_free != SQL_TRANSIENT)
-		return mem_set_strl_dynamic(mem, value, len_hint);
-}
-
 /** Copy string to a newly allocated memory. The MEM type becomes STRING. */
 int
 mem_copy_str(struct Mem *mem, const char *value, uint32_t len);
@@ -410,14 +362,6 @@ mem_copy_str(struct Mem *mem, const char *value, uint32_t len);
 int
 mem_copy_str0(struct Mem *mem, const char *value);
 
-static inline int
-mem_copy_strl(struct Mem *mem, const char *value, int len_hint)
-{
-	if (len_hint < 0)
-		return mem_copy_str0(mem, value);
-	return mem_copy_str(mem, value, len_hint);
-}
-
 /**
  * Clear MEM and set it to VARBINARY. The binary value belongs to another
  * object.
@@ -446,18 +390,6 @@ mem_set_bin_dynamic(struct Mem *mem, char *value, uint32_t size);
 void
 mem_set_bin_allocated(struct Mem *mem, char *value, uint32_t size);
 
-static inline void
-mem_set_binl(struct Mem *mem, char *value, uint32_t size,
-	     void (*custom_free)(void *))
-{
-	if (custom_free == SQL_STATIC)
-		return mem_set_bin_static(mem, value, size);
-	if (custom_free == SQL_DYNAMIC)
-		return mem_set_bin_allocated(mem, value, size);
-	if (custom_free != SQL_TRANSIENT)
-		return mem_set_bin_dynamic(mem, value, size);
-}
-
 /**
  * Copy binary value to a newly allocated memory. The MEM type becomes
  * VARBINARY.
@@ -905,9 +837,6 @@ mem_type_to_str(const struct Mem *p);
 enum mp_type
 mem_mp_type(const struct Mem *mem);
 
-enum mp_type
-sql_value_type(struct Mem *);
-
 #ifdef SQL_DEBUG
 int sqlVdbeCheckMemInvariants(struct Mem *);
 void sqlVdbeMemPrettyPrint(Mem * pMem, char *zBuf);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 72f349944..9ccbe4ea3 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -355,45 +355,6 @@ sql_stricmp(const char *, const char *);
 int
 sql_strnicmp(const char *, const char *, int);
 
-sql *
-sql_context_db_handle(sql_context *);
-
-
-void
-sql_result_blob(sql_context *, const void *,
-		    int, void (*)(void *));
-
-void
-sql_result_blob64(sql_context *, const void *,
-		      sql_uint64, void (*)(void *));
-
-void
-sql_result_double(sql_context *, double);
-
-void
-sql_result_uint(sql_context *ctx, uint64_t u_val);
-
-void
-sql_result_int(sql_context *ctx, int64_t val);
-
-void
-sql_result_bool(struct sql_context *ctx, bool value);
-
-void
-sql_result_null(sql_context *);
-
-void
-sql_result_text(sql_context *, const char *,
-		    int, void (*)(void *));
-
-void
-sql_result_text64(sql_context *, const char *,
-		      sql_uint64, void (*)(void *));
-
-void
-sql_result_value(sql_context *,
-		     sql_value *);
-
 char *
 sql_mprintf(const char *, ...);
 char *
@@ -567,14 +528,6 @@ sql_vfs_register(sql_vfs *, int makeDflt);
 void
 sql_unbind(struct sql_stmt *stmt);
 
-int
-sql_bind_blob(sql_stmt *, int, const void *,
-		  int n, void (*)(void *));
-
-int
-sql_bind_blob64(sql_stmt *, int, const void *,
-		    sql_uint64, void (*)(void *));
-
 int
 sql_bind_double(sql_stmt *, int, double);
 
@@ -602,8 +555,10 @@ int
 sql_bind_null(sql_stmt *, int);
 
 int
-sql_bind_text64(sql_stmt *, int, const char *,
-		    sql_uint64, void (*)(void *));
+sql_bind_str_static(sql_stmt *stmt, int i, const char *str, uint32_t len);
+
+int
+sql_bind_bin_static(sql_stmt *stmt, int i, const char *str, uint32_t size);
 
 int
 sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid);
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 8f7e28ffb..59d01da88 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -107,135 +107,6 @@ sql_metadata_is_full()
 	return current_session()->sql_flags & SQL_FullMetadata;
 }
 
-/**************************** sql_result_  ******************************
- * The following routines are used by user-defined functions to specify
- * the function result.
- *
- * The setStrOrError() function sets the result as a string or blob but
- * if the string or blob is too large, it then sets the error code.
- *
- * The invokeValueDestructor(P,X) routine invokes destructor function X()
- * on value P is not going to be used and need to be destroyed.
- */
-static void
-setResultStrOrError(sql_context * pCtx,	/* Function context */
-		    const char *z,	/* String pointer */
-		    int n,	/* Bytes in string, or negative */
-		    void (*xDel) (void *)	/* Destructor function */
-    )
-{
-	if (xDel != SQL_TRANSIENT)
-		return mem_set_strl(pCtx->pOut, (char *)z, n, xDel);
-	if (mem_copy_strl(pCtx->pOut, z, n) != 0)
-		pCtx->is_aborted = true;
-}
-
-static int
-invokeValueDestructor(const void *p,	/* Value to destroy */
-		      void (*xDel) (void *),	/* The destructor */
-		      sql_context *pCtx	/* Set an error if no NULL */
-    )
-{
-	assert(xDel != SQL_DYNAMIC);
-	if (xDel == 0) {
-		/* noop */
-	} else if (xDel == SQL_TRANSIENT) {
-		/* noop */
-	} else {
-		xDel((void *)p);
-	}
-	if (pCtx) {
-		diag_set(ClientError, ER_SQL_EXECUTE, "string or binary string"\
-			 "is too big");
-		pCtx->is_aborted = true;
-	}
-	return -1;
-}
-
-void
-sql_result_blob(sql_context * pCtx,
-		    const void *z, int n, void (*xDel) (void *)
-    )
-{
-	assert(n >= 0);
-	if (xDel != SQL_TRANSIENT)
-		mem_set_binl(pCtx->pOut, (char *)z, n, xDel);
-	else if (mem_copy_bin(pCtx->pOut, z, n) != 0)
-		pCtx->is_aborted = true;
-}
-
-void
-sql_result_blob64(sql_context * pCtx,
-		      const void *z, sql_uint64 n, void (*xDel) (void *)
-    )
-{
-	assert(xDel != SQL_DYNAMIC);
-	if (n > 0x7fffffff) {
-		(void)invokeValueDestructor(z, xDel, pCtx);
-	} else {
-		setResultStrOrError(pCtx, z, (int)n, xDel);
-	}
-}
-
-void
-sql_result_double(sql_context * pCtx, double rVal)
-{
-	mem_set_double(pCtx->pOut, rVal);
-}
-
-void
-sql_result_uint(sql_context *ctx, uint64_t u_val)
-{
-	mem_set_uint(ctx->pOut, u_val);
-}
-
-void
-sql_result_int(sql_context *ctx, int64_t val)
-{
-	mem_set_int(ctx->pOut, val, val < 0);
-}
-
-void
-sql_result_bool(struct sql_context *ctx, bool value)
-{
-	mem_set_bool(ctx->pOut, value);
-}
-
-void
-sql_result_null(sql_context * pCtx)
-{
-	mem_set_null(pCtx->pOut);
-}
-
-void
-sql_result_text(sql_context * pCtx,
-		    const char *z, int n, void (*xDel) (void *)
-    )
-{
-	setResultStrOrError(pCtx, z, n, xDel);
-}
-
-void
-sql_result_text64(sql_context * pCtx,
-		      const char *z,
-		      sql_uint64 n,
-		      void (*xDel) (void *))
-{
-	assert(xDel != SQL_DYNAMIC);
-	if (n > 0x7fffffff) {
-		(void)invokeValueDestructor(z, xDel, pCtx);
-	} else {
-		setResultStrOrError(pCtx, z, (int)n, xDel);
-	}
-}
-
-void
-sql_result_value(sql_context * pCtx, sql_value * pValue)
-{
-	if (mem_copy(pCtx->pOut, pValue) != 0)
-		pCtx->is_aborted = true;
-}
-
 /*
  * Execute the statement pStmt, either until a row of data is ready, the
  * statement is completely executed or an error occurs.
@@ -313,23 +184,6 @@ sql_step(sql_stmt * pStmt)
 	return sqlStep(v);
 }
 
-/*
- * Extract the user data from a sql_context structure and return a
- * pointer to it.
- *
- * IMPLEMENTATION-OF: R-46798-50301 The sql_context_db_handle() interface
- * returns a copy of the pointer to the database connection (the 1st
- * parameter) of the sql_create_function() and
- * sql_create_function16() routines that originally registered the
- * application defined function.
- */
-sql *
-sql_context_db_handle(sql_context * p)
-{
-	assert(p && p->pOut);
-	return p->pOut->db;
-}
-
 /*
  * Return the number of columns in the result set for the statement pStmt.
  */
@@ -579,73 +433,6 @@ sql_unbind(struct sql_stmt *stmt)
 	}
 }
 
-/*
- * Bind a text or BLOB value.
- */
-static int
-bindText(sql_stmt * pStmt,	/* The statement to bind against */
-	 int i,			/* Index of the parameter to bind */
-	 const void *zData,	/* Pointer to the data to be bound */
-	 int nData,		/* Number of bytes of data to be bound */
-	 void (*xDel) (void *)	/* Destructor for the data */
-    )
-{
-	Vdbe *p = (Vdbe *) pStmt;
-	Mem *pVar;
-	if (vdbeUnbind(p, i) != 0) {
-		if (xDel != SQL_STATIC && xDel != SQL_TRANSIENT)
-			xDel((void *)zData);
-		return -1;
-	}
-	if (zData == NULL)
-		return 0;
-	pVar = &p->aVar[i - 1];
-	if (xDel != SQL_TRANSIENT)
-		mem_set_strl(pVar, (char *)zData, nData, xDel);
-	else if (mem_copy_strl(pVar, zData, nData) != 0)
-		return -1;
-	return sql_bind_type(p, i, "text");
-}
-
-/*
- * Bind a blob value to an SQL statement variable.
- */
-int
-sql_bind_blob(sql_stmt * pStmt,
-		  int i, const void *zData, int nData, void (*xDel) (void *)
-    )
-{
-	struct Vdbe *p = (Vdbe *) pStmt;
-	if (vdbeUnbind(p, i) != 0) {
-		if (xDel != SQL_STATIC && xDel != SQL_TRANSIENT)
-			xDel((void *)zData);
-		return -1;
-	}
-	if (zData == NULL)
-		return 0;
-	struct Mem *var = &p->aVar[i - 1];
-	if (xDel != SQL_TRANSIENT)
-		mem_set_binl(var, (char *)zData, nData, xDel);
-	else if (mem_copy_bin(var, zData, nData) != 0)
-		return -1;
-	return sql_bind_type(p, i, "varbinary");
-}
-
-int
-sql_bind_blob64(sql_stmt * pStmt,
-		    int i,
-		    const void *zData,
-		    sql_uint64 nData, void (*xDel) (void *)
-    )
-{
-	assert(xDel != SQL_DYNAMIC);
-	if (nData > 0x7fffffff) {
-		return invokeValueDestructor(zData, xDel, 0);
-	} else {
-		return sql_bind_blob(pStmt, i, zData, (int)nData, xDel);
-	}
-}
-
 int
 sql_bind_double(sql_stmt * pStmt, int i, double rValue)
 {
@@ -719,18 +506,19 @@ sql_bind_ptr(struct sql_stmt *stmt, int i, void *ptr)
 }
 
 int
-sql_bind_text64(sql_stmt * pStmt,
-		    int i,
-		    const char *zData,
-		    sql_uint64 nData,
-		    void (*xDel) (void *))
-{
-	assert(xDel != SQL_DYNAMIC);
-	if (nData > 0x7fffffff) {
-		return invokeValueDestructor(zData, xDel, 0);
-	} else {
-		return bindText(pStmt, i, zData, (int)nData, xDel);
-	}
+sql_bind_str_static(sql_stmt *stmt, int i, const char *str, uint32_t len)
+{
+	struct Vdbe *vdbe = (struct Vdbe *)stmt;
+	mem_set_str_static(&vdbe->aVar[i - 1], (char *)str, len);
+	return sql_bind_type(vdbe, i, "text");
+}
+
+int
+sql_bind_bin_static(sql_stmt *stmt, int i, const char *str, uint32_t size)
+{
+	struct Vdbe *vdbe = (struct Vdbe *)stmt;
+	mem_set_bin_static(&vdbe->aVar[i - 1], (char *)str, size);
+	return sql_bind_type(vdbe, i, "text");
 }
 
 int
-- 
2.25.1

[Tarantool-patches] [PATCH v1 19/21] sql: remove MEM_Dyn flag

[Mergen Imeev via Tarantool-patches]

This patch removes the MEM_Dyn flag, because after changes in the SQL
built-in functions, this flag is no longer used.

Needed for #4145
---
 src/box/sql/mem.c | 108 +++++++---------------------------------------
 src/box/sql/mem.h |  53 +----------------------
 2 files changed, 16 insertions(+), 145 deletions(-)

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 895003580..a001ac8c5 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -207,7 +207,6 @@ mem_create(struct Mem *mem)
 	mem->szMalloc = 0;
 	mem->uTemp = 0;
 	mem->db = sql_get();
-	mem->xDel = NULL;
 #ifdef SQL_DEBUG
 	mem->pScopyFrom = NULL;
 	mem->pFiller = NULL;
@@ -217,15 +216,10 @@ mem_create(struct Mem *mem)
 static inline void
 mem_clear(struct Mem *mem)
 {
-	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 {
-			struct VdbeFrame *frame = mem->u.pFrame;
-			frame->pParent = frame->v->pDelFrame;
-			frame->v->pDelFrame = frame;
-		}
+	if (mem->type == MEM_TYPE_FRAME) {
+		struct VdbeFrame *frame = mem->u.pFrame;
+		frame->pParent = frame->v->pDelFrame;
+		frame->v->pDelFrame = frame;
 	}
 	mem->type = MEM_TYPE_NULL;
 	mem->flags = 0;
@@ -320,21 +314,14 @@ set_str_const(struct Mem *mem, char *value, uint32_t len, int alloc_type)
 static inline void
 set_str_dynamic(struct Mem *mem, char *value, uint32_t len, int alloc_type)
 {
-	assert((mem->flags & MEM_Dyn) == 0 || value != mem->z);
 	assert(mem->szMalloc == 0 || value != mem->zMalloc);
-	assert(alloc_type == MEM_Dyn || alloc_type == 0);
 	mem_destroy(mem);
 	mem->z = value;
 	mem->n = len;
 	mem->type = MEM_TYPE_STR;
 	mem->flags = alloc_type;
-	if (alloc_type == MEM_Dyn) {
-		mem->xDel = sql_free;
-	} else {
-		mem->xDel = NULL;
-		mem->zMalloc = mem->z;
-		mem->szMalloc = sqlDbMallocSize(mem->db, mem->zMalloc);
-	}
+	mem->zMalloc = mem->z;
+	mem->szMalloc = sqlDbMallocSize(mem->db, mem->zMalloc);
 }
 
 void
@@ -349,12 +336,6 @@ mem_set_str_static(struct Mem *mem, char *value, uint32_t len)
 	set_str_const(mem, value, len, MEM_Static);
 }
 
-void
-mem_set_str_dynamic(struct Mem *mem, char *value, uint32_t len)
-{
-	set_str_dynamic(mem, value, len, MEM_Dyn);
-}
-
 void
 mem_set_str_allocated(struct Mem *mem, char *value, uint32_t len)
 {
@@ -375,13 +356,6 @@ mem_set_str0_static(struct Mem *mem, char *value)
 	mem->flags |= MEM_Term;
 }
 
-void
-mem_set_str0_dynamic(struct Mem *mem, char *value)
-{
-	set_str_dynamic(mem, value, strlen(value), MEM_Dyn);
-	mem->flags |= MEM_Term;
-}
-
 void
 mem_set_str0_allocated(struct Mem *mem, char *value)
 {
@@ -436,21 +410,14 @@ set_bin_const(struct Mem *mem, char *value, uint32_t size, int alloc_type)
 static inline void
 set_bin_dynamic(struct Mem *mem, char *value, uint32_t size, int alloc_type)
 {
-	assert((mem->flags & MEM_Dyn) == 0 || value != mem->z);
 	assert(mem->szMalloc == 0 || value != mem->zMalloc);
-	assert(alloc_type == MEM_Dyn || alloc_type == 0);
 	mem_destroy(mem);
 	mem->z = value;
 	mem->n = size;
 	mem->type = MEM_TYPE_BIN;
 	mem->flags = alloc_type;
-	if (alloc_type == MEM_Dyn) {
-		mem->xDel = sql_free;
-	} else {
-		mem->xDel = NULL;
-		mem->zMalloc = mem->z;
-		mem->szMalloc = sqlDbMallocSize(mem->db, mem->zMalloc);
-	}
+	mem->zMalloc = mem->z;
+	mem->szMalloc = sqlDbMallocSize(mem->db, mem->zMalloc);
 }
 
 void
@@ -465,12 +432,6 @@ mem_set_bin_static(struct Mem *mem, char *value, uint32_t size)
 	set_bin_const(mem, value, size, MEM_Static);
 }
 
-void
-mem_set_bin_dynamic(struct Mem *mem, char *value, uint32_t size)
-{
-	set_bin_dynamic(mem, value, size, MEM_Dyn);
-}
-
 void
 mem_set_bin_allocated(struct Mem *mem, char *value, uint32_t size)
 {
@@ -524,13 +485,6 @@ mem_set_map_static(struct Mem *mem, char *value, uint32_t size)
 	set_msgpack_value(mem, value, size, MEM_Static, MEM_TYPE_MAP);
 }
 
-void
-mem_set_map_dynamic(struct Mem *mem, char *value, uint32_t size)
-{
-	assert(mp_typeof(*value) == MP_MAP);
-	set_msgpack_value(mem, value, size, MEM_Dyn, MEM_TYPE_MAP);
-}
-
 void
 mem_set_map_allocated(struct Mem *mem, char *value, uint32_t size)
 {
@@ -552,13 +506,6 @@ mem_set_array_static(struct Mem *mem, char *value, uint32_t size)
 	set_msgpack_value(mem, value, size, MEM_Static, MEM_TYPE_ARRAY);
 }
 
-void
-mem_set_array_dynamic(struct Mem *mem, char *value, uint32_t size)
-{
-	assert(mp_typeof(*value) == MP_ARRAY);
-	set_msgpack_value(mem, value, size, MEM_Dyn, MEM_TYPE_ARRAY);
-}
-
 void
 mem_set_array_allocated(struct Mem *mem, char *value, uint32_t size)
 {
@@ -2643,18 +2590,6 @@ mem_mp_type(const struct Mem *mem)
 int
 sqlVdbeCheckMemInvariants(Mem * p)
 {
-	/* If MEM_Dyn is set then Mem.xDel!=0.
-	 * Mem.xDel is might not be initialized if MEM_Dyn is clear.
-	 */
-	assert((p->flags & MEM_Dyn) == 0 || p->xDel != 0);
-
-	/* MEM_Dyn may only be set if Mem.szMalloc==0.  In this way we
-	 * ensure that if Mem.szMalloc>0 then it is safe to do
-	 * Mem.z = Mem.zMalloc without having to check Mem.flags&MEM_Dyn.
-	 * That saves a few cycles in inner loops.
-	 */
-	assert((p->flags & MEM_Dyn) == 0 || p->szMalloc == 0);
-
 	/* The szMalloc field holds the correct memory allocation size */
 	assert(p->szMalloc == 0 ||
 	       p->szMalloc == sqlDbMallocSize(p->db, p->zMalloc));
@@ -2669,7 +2604,6 @@ sqlVdbeCheckMemInvariants(Mem * p)
 	 */
 	if ((p->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0 && p->n > 0) {
 		assert(((p->szMalloc > 0 && p->z == p->zMalloc) ? 1 : 0) +
-		       ((p->flags & MEM_Dyn) != 0 ? 1 : 0) +
 		       ((p->flags & MEM_Ephem) != 0 ? 1 : 0) +
 		       ((p->flags & MEM_Static) != 0 ? 1 : 0) == 1);
 	}
@@ -2689,15 +2623,12 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
 	if (pMem->type == MEM_TYPE_BIN) {
 		int i;
 		char c;
-		if (f & MEM_Dyn) {
-			c = 'z';
-			assert((f & (MEM_Static|MEM_Ephem))==0);
-		} else if (f & MEM_Static) {
+		if (f & MEM_Static) {
 			c = 't';
-			assert((f & (MEM_Dyn|MEM_Ephem))==0);
+			assert((f & MEM_Ephem) == 0);
 		} else if (f & MEM_Ephem) {
 			c = 'e';
-			assert((f & (MEM_Static|MEM_Dyn))==0);
+			assert((f & MEM_Static) == 0);
 		} else {
 			c = 's';
 		}
@@ -2721,15 +2652,12 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
 	} else if (pMem->type == MEM_TYPE_STR) {
 		int j, k;
 		zBuf[0] = ' ';
-		if (f & MEM_Dyn) {
-			zBuf[1] = 'z';
-			assert((f & (MEM_Static|MEM_Ephem))==0);
-		} else if (f & MEM_Static) {
+		if (f & MEM_Static) {
 			zBuf[1] = 't';
-			assert((f & (MEM_Dyn|MEM_Ephem))==0);
+			assert((f & MEM_Ephem)==0);
 		} else if (f & MEM_Ephem) {
 			zBuf[1] = 'e';
-			assert((f & (MEM_Static|MEM_Dyn))==0);
+			assert((f & MEM_Static)==0);
 		} else {
 			zBuf[1] = 's';
 		}
@@ -2842,13 +2770,9 @@ sqlVdbeMemGrow(struct Mem *pMem, int n, int bPreserve)
 	if (bPreserve && pMem->z && pMem->z != pMem->zMalloc) {
 		memcpy(pMem->zMalloc, pMem->z, pMem->n);
 	}
-	if ((pMem->flags & MEM_Dyn) != 0) {
-		assert(pMem->xDel != 0 && pMem->xDel != SQL_DYNAMIC);
-		pMem->xDel((void *)(pMem->z));
-	}
 
 	pMem->z = pMem->zMalloc;
-	pMem->flags &= ~(MEM_Dyn | MEM_Ephem | MEM_Static);
+	pMem->flags &= ~(MEM_Ephem | MEM_Static);
 	return 0;
 }
 
@@ -2867,11 +2791,9 @@ int
 sqlVdbeMemClearAndResize(Mem * pMem, int szNew)
 {
 	assert(szNew > 0);
-	assert((pMem->flags & MEM_Dyn) == 0 || pMem->szMalloc == 0);
 	if (pMem->szMalloc < szNew) {
 		return sqlVdbeMemGrow(pMem, szNew, 0);
 	}
-	assert((pMem->flags & MEM_Dyn) == 0);
 	pMem->z = pMem->zMalloc;
 	return 0;
 }
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 1d995726f..76769835d 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -87,7 +87,6 @@ struct Mem {
 	int szMalloc;		/* Size of the zMalloc allocation */
 	u32 uTemp;		/* Transient storage for serial_type in OP_MakeRecord */
 	sql *db;		/* The associated database connection */
-	void (*xDel) (void *);	/* Destructor for Mem.z - only valid if MEM_Dyn */
 #ifdef SQL_DEBUG
 	Mem *pScopyFrom;	/* This Mem is a shallow copy of pScopyFrom */
 	void *pFiller;		/* So that sizeof(Mem) is a multiple of 8 */
@@ -106,7 +105,6 @@ struct Mem {
  * string is \000 or \u0000 terminated
  */
 #define MEM_Term      0x0400	/* String rep is nul terminated */
-#define MEM_Dyn       0x0800	/* Need to call Mem.xDel() on Mem.z */
 #define MEM_Static    0x1000	/* Mem.z points to a static string */
 #define MEM_Ephem     0x2000	/* Mem.z points to an ephemeral string */
 
@@ -217,13 +215,6 @@ mem_is_ephemeral(const struct Mem *mem)
 	return (mem->flags & MEM_Ephem) != 0;
 }
 
-static inline bool
-mem_is_dynamic(const struct Mem *mem)
-{
-	assert(mem_is_bytes(mem));
-	return (mem->flags & MEM_Dyn) != 0;
-}
-
 static inline bool
 mem_is_allocated(const struct Mem *mem)
 {
@@ -306,14 +297,6 @@ mem_set_str_ephemeral(struct Mem *mem, char *value, uint32_t len);
 void
 mem_set_str_static(struct Mem *mem, char *value, uint32_t len);
 
-/**
- * Clear MEM and set it to STRING. The string was allocated by another object
- * and passed to MEM. MEMs with this allocation type must free given memory
- * whenever the MEM changes.
- */
-void
-mem_set_str_dynamic(struct Mem *mem, char *value, uint32_t len);
-
 /**
  * Clear MEM and set it to STRING. The string was allocated by another object
  * and passed to MEM. MEMs with this allocation type only deallocate the string
@@ -334,14 +317,6 @@ mem_set_str0_ephemeral(struct Mem *mem, char *value);
 void
 mem_set_str0_static(struct Mem *mem, char *value);
 
-/**
- * Clear MEM and set it to NULL-terminated STRING. The string was allocated by
- * another object and passed to MEM. MEMs with this allocation type must free
- * given memory whenever the MEM changes.
- */
-void
-mem_set_str0_dynamic(struct Mem *mem, char *value);
-
 /**
  * Clear MEM and set it to NULL-terminated STRING. The string was allocated by
  * another object and passed to MEM. MEMs with this allocation type only
@@ -373,14 +348,6 @@ mem_set_bin_ephemeral(struct Mem *mem, char *value, uint32_t size);
 void
 mem_set_bin_static(struct Mem *mem, char *value, uint32_t size);
 
-/**
- * Clear MEM and set it to VARBINARY. The binary value was allocated by another
- * object and passed to MEM. MEMs with this allocation type must free given
- * memory whenever the MEM changes.
- */
-void
-mem_set_bin_dynamic(struct Mem *mem, char *value, uint32_t size);
-
 /**
  * Clear MEM and set it to VARBINARY. The binary value was allocated by another
  * object and passed to MEM. MEMs with this allocation type only deallocate the
@@ -411,14 +378,6 @@ mem_set_map_ephemeral(struct Mem *mem, char *value, uint32_t size);
 void
 mem_set_map_static(struct Mem *mem, char *value, uint32_t size);
 
-/**
- * Clear MEM and set it to MAP. The binary value was allocated by another object
- * and passed to MEM. The binary value must be msgpack of MAP type. MEMs with
- * this allocation type must free given memory whenever the MEM changes.
- */
-void
-mem_set_map_dynamic(struct Mem *mem, char *value, uint32_t size);
-
 /**
  * Clear MEM and set it to MAP. The binary value was allocated by another object
  * and passed to MEM. The binary value must be msgpack of MAP type. MEMs with
@@ -443,15 +402,6 @@ mem_set_array_ephemeral(struct Mem *mem, char *value, uint32_t size);
 void
 mem_set_array_static(struct Mem *mem, char *value, uint32_t size);
 
-/**
- * Clear MEM and set it to ARRAY. The binary value was allocated by another
- * object and passed to MEM. The binary value must be msgpack of ARRAY type.
- * MEMs with this allocation type must free given memory whenever the MEM
- * changes.
- */
-void
-mem_set_array_dynamic(struct Mem *mem, char *value, uint32_t size);
-
 /**
  * Clear MEM and set it to ARRAY. The binary value was allocated by another
  * object and passed to MEM. The binary value must be msgpack of ARRAY type.
@@ -868,8 +818,7 @@ int sqlVdbeMemTooBig(Mem *);
 /* Return TRUE if Mem X contains dynamically allocated content - anything
  * that needs to be deallocated to avoid a leak.
  */
-#define VdbeMemDynamic(X) (((X)->flags & MEM_Dyn) != 0 ||\
-			   ((X)->type & MEM_TYPE_FRAME) != 0)
+#define VdbeMemDynamic(X) (((X)->type & MEM_TYPE_FRAME) != 0)
 
 /**
  * Perform comparison of two tuples: unpacked (key1) and packed (key2)
-- 
2.25.1

[Tarantool-patches] [PATCH v1 20/21] sql: remove MEM_Term flag

[Mergen Imeev via Tarantool-patches]

This patch removes the MEM_Term flag, because after changes in the SQL
built-in functions, this flag is no longer used.

Needed for #4145
---
 src/box/sql/mem.c       | 78 +++--------------------------------------
 src/box/sql/mem.h       | 36 -------------------
 src/box/sql/printf.c    | 18 ++++++++--
 src/box/sql/vdbe.h      |  4 ++-
 src/box/sql/vdbeaux.c   | 31 ++++------------
 src/box/sql/whereexpr.c | 19 ++++++----
 6 files changed, 41 insertions(+), 145 deletions(-)

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index a001ac8c5..7951f9ad5 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -346,21 +346,18 @@ void
 mem_set_str0_ephemeral(struct Mem *mem, char *value)
 {
 	set_str_const(mem, value, strlen(value), MEM_Ephem);
-	mem->flags |= MEM_Term;
 }
 
 void
 mem_set_str0_static(struct Mem *mem, char *value)
 {
 	set_str_const(mem, value, strlen(value), MEM_Static);
-	mem->flags |= MEM_Term;
 }
 
 void
 mem_set_str0_allocated(struct Mem *mem, char *value)
 {
 	set_str_dynamic(mem, value, strlen(value), 0);
-	mem->flags |= MEM_Term;
 }
 
 int
@@ -392,7 +389,6 @@ mem_copy_str0(struct Mem *mem, const char *value)
 	if (mem_copy_str(mem, value, len + 1) != 0)
 		return -1;
 	mem->n = len;
-	mem->flags |= MEM_Term;
 	return 0;
 }
 
@@ -657,24 +653,12 @@ int_to_str0(struct Mem *mem)
 	return mem_copy_str0(mem, str);
 }
 
-static inline int
-str_to_str0(struct Mem *mem)
-{
-	assert(mem->type == MEM_TYPE_STR);
-	if (sqlVdbeMemGrow(mem, mem->n + 1, 1) != 0)
-		return -1;
-	mem->z[mem->n] = '\0';
-	mem->flags |= MEM_Term;
-	mem->flags &= ~MEM_Scalar;
-	return 0;
-}
-
 static inline int
 str_to_bin(struct Mem *mem)
 {
 	assert(mem->type == MEM_TYPE_STR);
 	mem->type = MEM_TYPE_BIN;
-	mem->flags &= ~(MEM_Term | MEM_Scalar);
+	mem->flags &= ~MEM_Scalar;
 	return 0;
 }
 
@@ -725,18 +709,6 @@ bin_to_str(struct Mem *mem)
 	return 0;
 }
 
-static inline int
-bin_to_str0(struct Mem *mem)
-{
-	assert(mem->type == MEM_TYPE_BIN);
-	if (sqlVdbeMemGrow(mem, mem->n + 1, 1) != 0)
-		return -1;
-	mem->z[mem->n] = '\0';
-	mem->type = MEM_TYPE_STR;
-	mem->flags = MEM_Term;
-	return 0;
-}
-
 static inline int
 bin_to_uuid(struct Mem *mem)
 {
@@ -1002,7 +974,7 @@ double_to_str0(struct Mem *mem)
 	sql_snprintf(BUF_SIZE, mem->z, "%!.15g", mem->u.r);
 	mem->n = strlen(mem->z);
 	mem->type = MEM_TYPE_STR;
-	mem->flags = MEM_Term;
+	mem->flags = 0;
 	return 0;
 }
 
@@ -1282,39 +1254,6 @@ mem_to_number(struct Mem *mem)
 	return -1;
 }
 
-int
-mem_to_str0(struct Mem *mem)
-{
-	assert(mem->type < MEM_TYPE_INVALID);
-	switch (mem->type) {
-	case MEM_TYPE_STR:
-		if ((mem->flags & MEM_Term) != 0) {
-			mem->flags &= ~MEM_Scalar;
-			return 0;
-		}
-		return str_to_str0(mem);
-	case MEM_TYPE_INT:
-	case MEM_TYPE_UINT:
-		return int_to_str0(mem);
-	case MEM_TYPE_DOUBLE:
-		return double_to_str0(mem);
-	case MEM_TYPE_BOOL:
-		return bool_to_str0(mem);
-	case MEM_TYPE_BIN:
-		return bin_to_str0(mem);
-	case MEM_TYPE_MAP:
-		return map_to_str0(mem);
-	case MEM_TYPE_ARRAY:
-		return array_to_str0(mem);
-	case MEM_TYPE_UUID:
-		return uuid_to_str0(mem);
-	case MEM_TYPE_DEC:
-		return dec_to_str0(mem);
-	default:
-		return -1;
-	}
-}
-
 int
 mem_to_str(struct Mem *mem)
 {
@@ -1761,15 +1700,6 @@ mem_get_bool(const struct Mem *mem, bool *b)
 	return -1;
 }
 
-int
-mem_get_str0(const struct Mem *mem, const char **s)
-{
-	if (mem->type != MEM_TYPE_STR || (mem->flags & MEM_Term) == 0)
-		return -1;
-	*s = mem->z;
-	return 0;
-}
-
 int
 mem_get_bin(const struct Mem *mem, const char **s)
 {
@@ -1812,7 +1742,7 @@ mem_copy(struct Mem *to, const struct Mem *from)
 	to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc);
 	memcpy(to->zMalloc, to->z, to->n);
 	to->z = to->zMalloc;
-	to->flags &= MEM_Term;
+	to->flags = 0;
 	return 0;
 }
 
@@ -1829,7 +1759,7 @@ mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from)
 		return;
 	if ((to->flags & (MEM_Static | MEM_Ephem)) != 0)
 		return;
-	to->flags &= MEM_Term;
+	to->flags = 0;
 	to->flags |= MEM_Ephem;
 	return;
 }
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 76769835d..e29807035 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -98,13 +98,6 @@ struct Mem {
 /** MEM is of SCALAR meta-type. */
 #define MEM_Scalar    0x0002
 #define MEM_Cleared   0x0200	/* NULL set by OP_Null, not from data */
-
-/* Whenever Mem contains a valid string or blob representation, one of
- * the following flags must be set to determine the memory management
- * policy for Mem.z.  The MEM_Term flag tells us whether or not the
- * string is \000 or \u0000 terminated
- */
-#define MEM_Term      0x0400	/* String rep is nul terminated */
 #define MEM_Static    0x1000	/* Mem.z points to a static string */
 #define MEM_Ephem     0x2000	/* Mem.z points to an ephemeral string */
 
@@ -603,14 +596,6 @@ mem_to_number(struct Mem *mem);
 int
 mem_to_str(struct Mem *mem);
 
-/**
- * Convert the given MEM to STRING. This function and the function above define
- * the rules that are used to convert values of all other types to STRING. In
- * this function, the string received after convertion is NULL-terminated.
- */
-int
-mem_to_str0(struct Mem *mem);
-
 /** Convert the given MEM to given type according to explicit cast rules. */
 int
 mem_cast_explicit(struct Mem *mem, enum field_type type);
@@ -715,27 +700,6 @@ mem_get_bool_unsafe(const struct Mem *mem)
 	return b;
 }
 
-/**
- * Return value for MEM of STRING type if MEM contains a NULL-terminated string.
- * Otherwise convert value of the MEM to NULL-terminated string if possible and
- * return converted value. Original MEM is not changed.
- */
-int
-mem_get_str0(const struct Mem *mem, const char **s);
-
-/**
- * Return value for MEM of STRING type if MEM contains NULL-terminated string.
- * Otherwise convert MEM to MEM of string type that contains NULL-terminated
- * string and return its value. Return NULL if conversion is impossible.
- */
-static inline const char *
-mem_as_str0(struct Mem *mem)
-{
-	if (mem_to_str0(mem) != 0)
-		return NULL;
-	return mem->z;
-}
-
 /**
  * Return value for MEM of VARBINARY type. For MEM of all other types convert
  * value of the MEM to VARBINARY if possible and return converted value.
diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
index b4ab0d0f9..f261a731c 100644
--- a/src/box/sql/printf.c
+++ b/src/box/sql/printf.c
@@ -160,8 +160,22 @@ getTextArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0;
-	struct Mem *mem = p->apArg[p->nUsed++];
-	return (char *)mem_as_str0(mem);
+	struct Mem mem;
+	mem_create(&mem);
+	mem_copy(&mem, p->apArg[p->nUsed++]);
+	if (mem_to_str(&mem) != 0) {
+		mem_destroy(&mem);
+		return NULL;
+	}
+	char *str = region_alloc(&fiber()->gc, mem.n + 1);
+	if (str == NULL) {
+		diag_set(OutOfMemory, mem.n + 1, "region", "str");
+		return NULL;
+	}
+	memcpy(str, mem.z, mem.n);
+	str[mem.n] = '\0';
+	mem_destroy(&mem);
+	return str;
 }
 
 /*
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index e40a1a0b3..89c83c43a 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -256,12 +256,14 @@ vdbe_metadata_set_col_autoincrement(struct Vdbe *p, int idx);
 int
 vdbe_metadata_set_col_span(struct Vdbe *p, int idx, const char *span);
 
+const struct Mem *
+vdbe_get_bound_value(struct Vdbe *vdbe, int id);
+
 void sqlVdbeCountChanges(Vdbe *);
 sql *sqlVdbeDb(Vdbe *);
 void sqlVdbeSetSql(Vdbe *, const char *z, int n);
 void sqlVdbeSwap(Vdbe *, Vdbe *);
 VdbeOp *sqlVdbeTakeOpArray(Vdbe *, int *, int *);
-sql_value *sqlVdbeGetBoundValue(Vdbe *, int);
 char *sqlVdbeExpandSql(Vdbe *, const char *);
 
 /**
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 3015760e1..a9ad7f50f 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2290,31 +2290,12 @@ sqlVdbeDb(Vdbe * v)
 	return v->db;
 }
 
-/*
- * Return a pointer to an sql_value structure containing the value bound
- * parameter iVar of VM v. Except, if the value is an SQL NULL, return
- * 0 instead. Unless it is NULL, apply type to the value before returning it.
- *
- * The returned value must be freed by the caller using sqlValueFree().
- */
-sql_value *
-sqlVdbeGetBoundValue(struct Vdbe *v, int iVar)
-{
-	assert(iVar > 0);
-	if (v) {
-		Mem *pMem = &v->aVar[iVar - 1];
-		if (!mem_is_null(pMem)) {
-			sql_value *pRet = sqlValueNew(v->db);
-			if (pRet == NULL)
-				return NULL;
-			if (mem_copy(pRet, pMem) != 0) {
-				sqlValueFree(pRet);
-				return NULL;
-			}
-			return pRet;
-		}
-	}
-	return 0;
+const struct Mem *
+vdbe_get_bound_value(struct Vdbe *vdbe, int id)
+{
+	if (vdbe == NULL || id < 0 || id >= vdbe->nVar)
+		return NULL;
+	return &vdbe->aVar[id];
 }
 
 void
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 6849f13ec..ad064d500 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -268,7 +268,7 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 	int cnt;
 	/* Database connection. */
 	sql *db = pParse->db;
-	sql_value *pVal = 0;
+	const struct Mem *var = NULL;
 	/* Opcode of pRight. */
 	int op;
 	/* Result code to return. */
@@ -307,12 +307,18 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 
 	op = pRight->op;
 	if (op == TK_VARIABLE) {
-		Vdbe *pReprepare = pParse->pReprepare;
-		int iCol = pRight->iColumn;
-		pVal = sqlVdbeGetBoundValue(pReprepare, iCol);
-		if (pVal != NULL && mem_is_str(pVal)) {
-			if (mem_as_str0(pVal) == NULL)
+		var = vdbe_get_bound_value(pParse->pReprepare,
+					   pRight->iColumn - 1);
+		if (var != NULL && mem_is_str(var)) {
+			uint32_t size = var->n + 1;
+			char *str = region_alloc(&fiber()->gc, size);
+			if (str == NULL) {
+				diag_set(OutOfMemory, size, "region", "str");
 				return -1;
+			}
+			memcpy(str, var->z, var->n);
+			str[var->n] = '\0';
+			z = str;
 		}
 		assert(pRight->op == TK_VARIABLE || pRight->op == TK_REGISTER);
 	} else if (op == TK_STRING) {
@@ -357,7 +363,6 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 	}
 
 	rc = (z != 0);
-	sqlValueFree(pVal);
 	return rc;
 }
 
-- 
2.25.1

[Tarantool-patches] [PATCH v1 21/21] sql: make arguments to be const

[Mergen Imeev via Tarantool-patches]

This patch forces SQL built-in function implementations to accept
'const struct Mem *' instead of just 'struct Mem *'.

Needed for #4145
---
 src/box/sql/func.c   | 92 ++++++++++++++++++++++----------------------
 src/box/sql/sqlInt.h |  5 ++-
 2 files changed, 50 insertions(+), 47 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 4e7404337..f1f063cb3 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -55,7 +55,7 @@ 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, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
@@ -70,7 +70,7 @@ step_sum(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** 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, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
@@ -95,7 +95,7 @@ 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, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
@@ -148,7 +148,7 @@ 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, const struct Mem *argv)
 {
 	assert(argc == 0 || argc == 1);
 	if (mem_is_null(ctx->pOut))
@@ -171,7 +171,7 @@ 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, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
@@ -204,7 +204,7 @@ 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, const struct Mem *argv)
 {
 	assert(argc == 1 || argc == 2);
 	(void)argc;
@@ -239,11 +239,11 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementations of the ABS() function. */
 static void
-func_abs_int(struct sql_context *ctx, int argc, struct Mem *argv)
+func_abs_int(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	struct Mem *arg = &argv[0];
+	const struct Mem *arg = &argv[0];
 	if (mem_is_null(arg))
 		return;
 	assert(mem_is_int(arg));
@@ -252,11 +252,11 @@ func_abs_int(struct sql_context *ctx, int argc, struct Mem *argv)
 }
 
 static void
-func_abs_double(struct sql_context *ctx, int argc, struct Mem *argv)
+func_abs_double(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	struct Mem *arg = &argv[0];
+	const struct Mem *arg = &argv[0];
 	if (mem_is_null(arg))
 		return;
 	assert(mem_is_double(arg));
@@ -284,11 +284,11 @@ utf8_len_str(const char *str, uint32_t size)
 }
 
 static void
-func_char_length(struct sql_context *ctx, int argc, struct Mem *argv)
+func_char_length(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	struct Mem *arg = &argv[0];
+	const struct Mem *arg = &argv[0];
 	if (mem_is_null(arg))
 		return;
 	assert(mem_is_str(arg) && arg->n >= 0);
@@ -297,11 +297,11 @@ func_char_length(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the UPPER() and LOWER() functions. */
 static void
-func_lower_upper(struct sql_context *ctx, int argc, struct Mem *argv)
+func_lower_upper(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	struct Mem *arg = &argv[0];
+	const struct Mem *arg = &argv[0];
 	if (mem_is_null(arg))
 		return;
 	assert(mem_is_str(arg) && arg->n >= 0);
@@ -350,7 +350,7 @@ func_lower_upper(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the NULLIF() function. */
 static void
-func_nullif(struct sql_context *ctx, int argc, struct Mem *argv)
+func_nullif(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 2);
 	(void)argc;
@@ -406,7 +406,7 @@ trim_bin_start(const char *str, int end, const char *octets, int octets_size,
 }
 
 static void
-func_trim_bin(struct sql_context *ctx, int argc, struct Mem *argv)
+func_trim_bin(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	if (mem_is_null(&argv[0]) || (argc == 3 && mem_is_null(&argv[2])))
 		return;
@@ -485,7 +485,7 @@ trim_str_start(const char *str, int end, const char *chars, uint8_t *chars_len,
 }
 
 static void
-func_trim_str(struct sql_context *ctx, int argc, struct Mem *argv)
+func_trim_str(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	if (mem_is_null(&argv[0]) || (argc == 3 && mem_is_null(&argv[2])))
 		return;
@@ -532,7 +532,7 @@ func_trim_str(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the POSITION() function. */
 static void
-func_position_octets(struct sql_context *ctx, int argc, struct Mem *argv)
+func_position_octets(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 2);
 	(void)argc;
@@ -555,7 +555,8 @@ func_position_octets(struct sql_context *ctx, int argc, struct Mem *argv)
 }
 
 static void
-func_position_characters(struct sql_context *ctx, int argc, struct Mem *argv)
+func_position_characters(struct sql_context *ctx, int argc,
+			 const struct Mem *argv)
 {
 	assert(argc == 2);
 	(void)argc;
@@ -629,7 +630,7 @@ substr_normalize(int64_t base_start, bool is_start_neg, uint64_t base_length,
 }
 
 static void
-func_substr_octets(struct sql_context *ctx, int argc, struct Mem *argv)
+func_substr_octets(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 2 || argc == 3);
 	if (mem_is_any_null(&argv[0], &argv[1]))
@@ -677,7 +678,8 @@ func_substr_octets(struct sql_context *ctx, int argc, struct Mem *argv)
 }
 
 static void
-func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv)
+func_substr_characters(struct sql_context *ctx, int argc, const
+		       struct Mem *argv)
 {
 	assert(argc == 2 || argc == 3);
 	(void)argc;
@@ -740,7 +742,7 @@ func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the CHAR() function. */
 static void
-func_char(struct sql_context *ctx, int argc, struct Mem *argv)
+func_char(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	if (argc == 0)
 		return mem_set_str_static(ctx->pOut, "", 0);
@@ -779,7 +781,7 @@ func_char(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the GREATEST() and LEAST() functions. */
 static void
-func_greatest_least(struct sql_context *ctx, int argc, struct Mem *argv)
+func_greatest_least(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc > 1);
 	int mask = ctx->func->def->name[0] == 'G' ? -1 : 0;
@@ -807,11 +809,11 @@ static const char hexdigits[] = {
 };
 
 static void
-func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
+func_hex(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	struct Mem *arg = &argv[0];
+	const struct Mem *arg = &argv[0];
 	if (mem_is_null(arg))
 		return mem_set_null(ctx->pOut);
 
@@ -835,11 +837,11 @@ func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the OCTET_LENGTH() function. */
 static void
-func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)
+func_octet_length(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	struct Mem *arg = &argv[0];
+	const struct Mem *arg = &argv[0];
 	if (mem_is_null(arg))
 		return;
 	assert(mem_is_bytes(arg) && arg->n >= 0);
@@ -848,7 +850,7 @@ func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the PRINTF() function. */
 static void
-func_printf(struct sql_context *ctx, int argc, struct Mem *argv)
+func_printf(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	if (argc < 1 || mem_is_null(&argv[0]))
 		return;
@@ -882,7 +884,7 @@ func_printf(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the RANDOM() function. */
 static void
-func_random(struct sql_context *ctx, int argc, struct Mem *argv)
+func_random(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	(void)argc;
 	(void)argv;
@@ -893,11 +895,11 @@ func_random(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the RANDOMBLOB() function. */
 static void
-func_randomblob(struct sql_context *ctx, int argc, struct Mem *argv)
+func_randomblob(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	struct Mem *arg = &argv[0];
+	const struct Mem *arg = &argv[0];
 	assert(mem_is_null(arg) || mem_is_int(arg));
 	if (mem_is_null(arg) || !mem_is_uint(arg))
 		return;
@@ -915,11 +917,11 @@ func_randomblob(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the ZEROBLOB() function. */
 static void
-func_zeroblob(struct sql_context *ctx, int argc, struct Mem *argv)
+func_zeroblob(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	struct Mem *arg = &argv[0];
+	const struct Mem *arg = &argv[0];
 	assert(mem_is_null(arg) || mem_is_int(arg));
 	if (mem_is_null(arg) || !mem_is_uint(arg))
 		return;
@@ -936,7 +938,7 @@ func_zeroblob(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the TYPEOF() function. */
 static void
-func_typeof(struct sql_context *ctx, int argc, struct Mem *argv)
+func_typeof(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
@@ -945,7 +947,7 @@ func_typeof(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the ROUND() function. */
 static void
-func_round(struct sql_context *ctx, int argc, struct Mem *argv)
+func_round(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1 || argc == 2);
 	if (mem_is_null(&argv[0]) || (argc == 2 && mem_is_null(&argv[1])))
@@ -971,7 +973,7 @@ func_round(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the ROW_COUNT() function. */
 static void
-func_row_count(struct sql_context *ctx, int argc, struct Mem *argv)
+func_row_count(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	(void)argc;
 	(void)argv;
@@ -981,7 +983,7 @@ func_row_count(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the UUID() function. */
 static void
-func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
+func_uuid(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	if (argc == 1) {
 		if (mem_is_null(&argv[0]))
@@ -1000,7 +1002,7 @@ func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the VERSION() function. */
 static void
-func_version(struct sql_context *ctx, int argc, struct Mem *argv)
+func_version(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	(void)argc;
 	(void)argv;
@@ -1009,7 +1011,7 @@ func_version(struct sql_context *ctx, int argc, struct Mem *argv)
 
 /** Implementation of the UNICODE() function. */
 static void
-func_unicode(struct sql_context *ctx, int argc, struct Mem *argv)
+func_unicode(struct sql_context *ctx, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
@@ -1240,7 +1242,7 @@ sql_utf8_pattern_compare(const char *pattern,
  * is NULL then result is NULL as well.
  */
 static void
-likeFunc(sql_context *context, int argc, struct Mem *argv)
+likeFunc(sql_context *context, int argc, const struct Mem *argv)
 {
 	u32 escape = SQL_END_OF_STRING;
 	int nPat;
@@ -1307,7 +1309,7 @@ likeFunc(sql_context *context, int argc, struct Mem *argv)
  * single-quote escapes.
  */
 static void
-quoteFunc(struct sql_context *context, int argc, struct Mem *argv)
+quoteFunc(struct sql_context *context, int argc, const struct Mem *argv)
 {
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
@@ -1392,7 +1394,7 @@ quoteFunc(struct sql_context *context, int argc, struct Mem *argv)
  * must be exact.  Collating sequences are not used.
  */
 static void
-replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
+replaceFunc(struct sql_context *context, int argc, const struct Mem *argv)
 {
 	const unsigned char *zStr;	/* The input string A */
 	const unsigned char *zPattern;	/* The pattern string B */
@@ -1466,7 +1468,7 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
  * soundex encoding of the string X.
  */
 static void
-soundexFunc(struct sql_context *context, int argc, struct Mem *argv)
+soundexFunc(struct sql_context *context, int argc, const struct Mem *argv)
 {
 	(void) argc;
 	char zResult[8];
@@ -1539,7 +1541,7 @@ func_sql_builtin_call_stub(struct func *func, struct port *args,
 }
 
 static void
-sql_builtin_stub(sql_context *ctx, int argc, struct Mem *argv)
+sql_builtin_stub(sql_context *ctx, int argc, const struct Mem *argv)
 {
 	(void) argc; (void) argv;
 	diag_set(ClientError, ER_SQL_EXECUTE,
@@ -1643,7 +1645,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, struct Mem *argv);
+	void (*call)(sql_context *ctx, int argc, const struct Mem *argv);
 	/** Call finalization function for this implementation. */
 	int (*finalize)(struct Mem *mem);
 };
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 9ccbe4ea3..51327f4fd 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2460,7 +2460,8 @@ int sqlIsNaN(double);
 struct PrintfArguments {
 	int nArg;		/* Total number of arguments */
 	int nUsed;		/* Number of arguments used so far */
-	sql_value **apArg;	/* The argument values */
+	/** The argument values. */
+	const sql_value **apArg;
 };
 
 void sqlVXPrintf(StrAccum *, const char *, va_list);
@@ -4303,7 +4304,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)(struct sql_context *ctx, int argc, struct Mem *argv);
+	void (*call)(struct sql_context *ctx, int argc, const struct Mem *argv);
 	/**
 	 * A VDBE-memory-compatible finalize method
 	 * (is valid only for aggregate function).
-- 
2.25.1

Re: [Tarantool-patches] [PATCH v1 01/21] sql: refactor CHAR() function

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

Before this commit on the branch I see a comment called 'Fix'.
Please, cleanup the branch from unfinished work.

On 08.10.2021 19:31, imeevma@tarantool.org wrote:
> Part of #4145
> ---
>  src/box/sql/func.c | 85 ++++++++++++++++++++++------------------------
>  1 file changed, 40 insertions(+), 45 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index a3c7d8d20..dd5e7d785 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -738,6 +738,45 @@ func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv)
>  		ctx->is_aborted = true;
>  }
>  
> +/** Implementation of the CHAR() function. */

Please, keep the comments explaining what the non-trivial functions do.

> +static void
> +func_char(struct sql_context *ctx, int argc, struct Mem *argv)
> +{

Re: [Tarantool-patches] [PATCH v1 02/21] sql: refactor GREATEST() and LEAST() functions

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for working on this!

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index dd5e7d785..7886f5f40 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -777,6 +777,29 @@ func_char(struct sql_context *ctx, int argc, struct Mem *argv)
>  	mem_set_str_allocated(ctx->pOut, str, (char *)ptr - str);
>  }
>  
> +/** Implementation of the GREATEST() and LEAST() functions. */
> +static void
> +func_greatest_least(struct sql_context *ctx, int argc, struct Mem *argv)
> +{
> +	assert(argc > 1);
> +	int mask = ctx->func->def->name[0] == 'G' ? -1 : 0;
> +	assert(ctx->func->def->name[0] == 'G' ||
> +	       ctx->func->def->name[0] == 'L');

Why don't you use the flag SQL_FUNC_MAX like it was done before?

> +
> +	if (mem_is_null(&argv[0]))
> +		return;
> +	int best = 0;
> +	for (int i = 1; i < argc; ++i) {
> +		if (mem_is_null(&argv[i]))
> +			return;
> +		int cmp = mem_cmp_scalar(&argv[best], &argv[i], ctx->coll);
> +		if ((cmp ^ mask) >= 0)
> +			best = i;
> +	}
> +	if (mem_copy(ctx->pOut, &argv[best]) != 0)
> +		ctx->is_aborted = true;
> +}

Re: [Tarantool-patches] [PATCH v1 03/21] sql: refactor HEX() function

[Vladislav Shpilevoy via Tarantool-patches]

Good job on the patch!

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 7886f5f40..e5d763be1 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -800,6 +800,39 @@ func_greatest_least(struct sql_context *ctx, int argc, struct Mem *argv)
>  		ctx->is_aborted = true;
>  }
>  
> +/** Implementation of the HEX() function. */
> +static const char hexdigits[] = {
> +	'0', '1', '2', '3', '4', '5', '6', '7',
> +	'8', '9', 'A', 'B', 'C', 'D', 'E', 'F'
> +};
> +
> +static void
> +func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
> +{
> +	assert(argc == 1);
> +	(void)argc;
> +	struct Mem *arg = &argv[0];
> +	if (mem_is_null(arg))
> +		return mem_set_null(ctx->pOut);

Isn't pOut already null? I thought it is created as null.

Re: [Tarantool-patches] [PATCH v1 04/21] sql: refactor LENGTH() function

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

On 08.10.2021 19:31, imeevma@tarantool.org wrote:
> Part of #4145
> ---
>  src/box/sql/func.c | 55 +++++++++++++---------------------------------
>  1 file changed, 15 insertions(+), 40 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index e5d763be1..863dbf1c4 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -833,6 +833,19 @@ func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
>  	mem_set_str_allocated(ctx->pOut, str, size);
>  }
>  
> +/** Implementation of the OCTET_LENGTH() function. */
> +static void
> +func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)

Why is the function LENGTH defined as 'octet_length' instead of just 'length'?

Re: [Tarantool-patches] [PATCH v1 05/21] sql: refactor PRINTF() function

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 863dbf1c4..f5040fb6e 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -846,6 +846,40 @@ func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)
>  	mem_set_uint(ctx->pOut, arg->n);
>  }
>  
> +/** Implementation of the PRINTF() function. */
> +static void
> +func_printf(struct sql_context *ctx, int argc, struct Mem *argv)
> +{
> +	if (argc < 1 || mem_is_null(&argv[0]))
> +		return;
> +	if (argc == 1 || !mem_is_str(&argv[0])) {
> +		struct Mem *mem = ctx->pOut;
> +		if (mem_copy(mem, &argv[0]) != 0 || mem_to_str(mem) != 0)
> +			ctx->is_aborted = true;
> +		return;
> +	}
> +	struct PrintfArguments pargs;
> +	struct StrAccum acc;
> +	char *format = argv[0].z;
> +	struct sql *db = sql_get();
> +
> +	pargs.nArg = argc - 1;
> +	pargs.nUsed = 0;
> +	pargs.apArg = sqlDbMallocRawNN(db, (argc - 1) * sizeof(*pargs.apArg));
> +	if (pargs.apArg == NULL) {
> +		ctx->is_aborted = true;
> +		return;
> +	}
> +	for (int i = 1; i < argc; ++i)
> +		pargs.apArg[i - 1] = &argv[i];
> +	sqlStrAccumInit(&acc, db, 0, 0, db->aLimit[SQL_LIMIT_LENGTH]);
> +	acc.printfFlags = SQL_PRINTF_SQLFUNC;
> +	sqlXPrintf(&acc, format, &pargs);
> +	sqlDbFree(db, pargs.apArg);
> +	if (mem_copy_str(ctx->pOut, sqlStrAccumFinish(&acc), acc.nChar) != 0)

It leaks now, because sqlStrAccumFinish is not destroyed. Previously it
was 'moved' into the mem via SQL_DYNAMIC. But now you copy it and the
original is not freed.

Re: [Tarantool-patches] [PATCH v1 14/21] sql: refactor UNICODE() function

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index fb7fd772e..5e12ef729 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -1007,6 +1007,19 @@ func_version(struct sql_context *ctx, int argc, struct Mem *argv)
>  	return mem_set_str0_static(ctx->pOut, (char *)tarantool_version());
>  }
>  
> +/** Implementation of the UNICODE() function. */
> +static void
> +func_unicode(struct sql_context *ctx, int argc, struct Mem *argv)
> +{
> +	assert(argc == 1);
> +	(void)argc;
> +	if (mem_is_null(&argv[0]))
> +		return;
> +	assert(mem_is_str(&argv[0]));
> +	const char *str = tt_cstr(argv[0].z, argv[0].n);
> +	mem_set_uint(ctx->pOut, sqlUtf8Read((const unsigned char **)&str));

You can dodge the copying. See utf8_next() in utf8.c:

	UChar32 c;
	U8_NEXT(str, pos, len, c);

Re: [Tarantool-patches] [PATCH v1 16/21] sql: refactor REPLACE() function

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

On 08.10.2021 19:32, Mergen Imeev via Tarantool-patches wrote:
> Part of #4145
> ---
>  src/box/sql/func.c | 58 ++++++++++++++++------------------------------
>  1 file changed, 20 insertions(+), 38 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index a3ca53545..f26d101e9 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -1460,34 +1460,26 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
>  	int i, j;		/* Loop counters */
>  
>  	assert(argc == 3);
> -	UNUSED_PARAMETER(argc);
> -	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]);
> -	if (zPattern == 0) {
> -		assert(mem_is_null(&argv[1])
> -		       || sql_context_db_handle(context)->mallocFailed);
> +	(void)argc;
> +	if (mem_is_any_null(&argv[0], &argv[1]) || mem_is_null(&argv[2]))
>  		return;
> -	}
> -	nPattern = mem_len_unsafe(&argv[1]);
> +	assert(mem_is_bytes);

1. You forgot to call the function.

> +	zStr = (const unsigned char *)argv[0].z;
> +	nStr = argv[0].n;
> +	zPattern = (const unsigned char *)argv[1].z;
> +	nPattern = argv[1].n;
>  	if (nPattern == 0) {
> -		assert(!mem_is_null(&argv[1]));
> -		sql_result_value(context, &argv[0]);
> +		if (mem_copy(context->pOut, &argv[0]) != 0)
> +			context->is_aborted = true;
>  		return;
> @@ -1497,22 +1489,12 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
>  			zOut[j++] = zStr[i];
>  		} else {
>  			u8 *zOld;
> -			sql *db = sql_context_db_handle(context);
>  			nOut += nRep - nPattern;
> -			testcase(nOut - 1 == db->aLimit[SQL_LIMIT_LENGTH]);
> -			testcase(nOut - 2 == db->aLimit[SQL_LIMIT_LENGTH]);
> -			if (nOut - 1 > db->aLimit[SQL_LIMIT_LENGTH]) {

2. Why did you drop length limit assertions and checks?

> -				diag_set(ClientError, ER_SQL_EXECUTE, "string "\
> -					 "or binary string is too big");
> -				context->is_aborted = true;
> -				sql_free(zOut);
> -				return;
> -			}

Re: [Tarantool-patches] [PATCH v1 19/21] sql: remove MEM_Dyn flag

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 895003580..a001ac8c5 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -2689,15 +2623,12 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
>  	if (pMem->type == MEM_TYPE_BIN) {
>  		int i;
>  		char c;
> -		if (f & MEM_Dyn) {
> -			c = 'z';
> -			assert((f & (MEM_Static|MEM_Ephem))==0);
> -		} else if (f & MEM_Static) {
> +		if (f & MEM_Static) {

1. != 0.

>  			c = 't';
> -			assert((f & (MEM_Dyn|MEM_Ephem))==0);
> +			assert((f & MEM_Ephem) == 0);
>  		} else if (f & MEM_Ephem) {
>  			c = 'e';
> -			assert((f & (MEM_Static|MEM_Dyn))==0);
> +			assert((f & MEM_Static) == 0);
>  		} else {
>  			c = 's';
>  		}
> @@ -2721,15 +2652,12 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
>  	} else if (pMem->type == MEM_TYPE_STR) {
>  		int j, k;
>  		zBuf[0] = ' ';
> -		if (f & MEM_Dyn) {
> -			zBuf[1] = 'z';
> -			assert((f & (MEM_Static|MEM_Ephem))==0);
> -		} else if (f & MEM_Static) {
> +		if (f & MEM_Static) {

2. != 0.

>  			zBuf[1] = 't';
> -			assert((f & (MEM_Dyn|MEM_Ephem))==0);
> +			assert((f & MEM_Ephem)==0);
>  		} else if (f & MEM_Ephem) {
>  			zBuf[1] = 'e';
> -			assert((f & (MEM_Static|MEM_Dyn))==0);
> +			assert((f & MEM_Static)==0);

3. Spaces around == here and above.

Re: [Tarantool-patches] [PATCH v1 20/21] sql: remove MEM_Term flag

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

> diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
> index b4ab0d0f9..f261a731c 100644
> --- a/src/box/sql/printf.c
> +++ b/src/box/sql/printf.c
> @@ -160,8 +160,22 @@ getTextArg(PrintfArguments * p)
>  {
>  	if (p->nArg <= p->nUsed)
>  		return 0;
> -	struct Mem *mem = p->apArg[p->nUsed++];
> -	return (char *)mem_as_str0(mem);
> +	struct Mem mem;
> +	mem_create(&mem);
> +	mem_copy(&mem, p->apArg[p->nUsed++]);

1. mem_copy_as_ephemeral() would be enough. But would be even
better to have mem_snprintf() which would work like snprintf -
save string representation of mem into a provided buffer and
return how many bytes written / need to be written. This though
could be postponed indefinitely, I don't mind.

> +	if (mem_to_str(&mem) != 0) {
> +		mem_destroy(&mem);
> +		return NULL;
> +	}
> +	char *str = region_alloc(&fiber()->gc, mem.n + 1);

2. printf is called for each row. Which means it will leak the
region. You can either do the snprintf-thing described above to
write directly into the needed buffer, or at least cleanup the
region using a save point in the end of sqlVXPrintf().

> diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
> index 6849f13ec..ad064d500 100644
> --- a/src/box/sql/whereexpr.c
> +++ b/src/box/sql/whereexpr.c
> @@ -307,12 +307,18 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
>  
>  	op = pRight->op;
>  	if (op == TK_VARIABLE) {
> -		Vdbe *pReprepare = pParse->pReprepare;
> -		int iCol = pRight->iColumn;
> -		pVal = sqlVdbeGetBoundValue(pReprepare, iCol);
> -		if (pVal != NULL && mem_is_str(pVal)) {
> -			if (mem_as_str0(pVal) == NULL)
> +		var = vdbe_get_bound_value(pParse->pReprepare,
> +					   pRight->iColumn - 1);
> +		if (var != NULL && mem_is_str(var)) {
> +			uint32_t size = var->n + 1;
> +			char *str = region_alloc(&fiber()->gc, size);

3. You leak the fiber's region. AFAIR, it is not cleaned up in the
end of parsing. Try to use pParse->region.

> +			if (str == NULL) {
> +				diag_set(OutOfMemory, size, "region", "str");
>  				return -1;
> +			}
> +			memcpy(str, var->z, var->n);
> +			str[var->n] = '\0';
> +			z = str;
>  		}

Re: [Tarantool-patches] [PATCH v1 01/21] sql: refactor CHAR() function

[Mergen Imeev via Tarantool-patches]

Hi! Thank you for the review! My answers, diff and new patch below. Also, I
replaced part of old code with ICU macro and changed commit-message.

On Fri, Oct 15, 2021 at 12:42:22AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> Before this commit on the branch I see a comment called 'Fix'.
> Please, cleanup the branch from unfinished work.
> 
Understood, fixed.

> On 08.10.2021 19:31, imeevma@tarantool.org wrote:
> > Part of #4145
> > ---
> >  src/box/sql/func.c | 85 ++++++++++++++++++++++------------------------
> >  1 file changed, 40 insertions(+), 45 deletions(-)
> > 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index a3c7d8d20..dd5e7d785 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -738,6 +738,45 @@ func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv)
> >  		ctx->is_aborted = true;
> >  }
> >  
> > +/** Implementation of the CHAR() function. */
> 
> Please, keep the comments explaining what the non-trivial functions do.
> 
Added comments for some functions. You will these diffs in this and next few
letters.

> > +static void
> > +func_char(struct sql_context *ctx, int argc, struct Mem *argv)
> > +{


Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 6b5099826..dee28b852 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -717,43 +717,55 @@ func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv)
 		ctx->is_aborted = true;
 }
 
-/** Implementation of the CHAR() function. */
+/**
+ * Implementation of the CHAR() function.
+ *
+ * This function takes zero or more arguments, each of which is an integer. It
+ * constructs a string where each character of the string is the unicode
+ * character for the corresponding integer argument.
+ *
+ * If an argument is negative or greater than 0x10ffff, the symbol "�" is used.
+ * Symbol '\0' used instead of NULL argument.
+ */
 static void
 func_char(struct sql_context *ctx, int argc, struct Mem *argv)
 {
 	if (argc == 0)
 		return mem_set_str_static(ctx->pOut, "", 0);
-	char *str = sqlDbMallocRawNN(sql_get(), argc * 4);
-	if (str == NULL) {
+	struct region *region = &fiber()->gc;
+	size_t svp = region_used(region);
+	UChar32 *buf = region_alloc(region, argc * sizeof(*buf));
+	if (buf == NULL) {
 		ctx->is_aborted = true;
 		return;
 	}
-	uint8_t *ptr = (uint8_t *)str;
+	int len = 0;
 	for (int i = 0; i < argc; ++i) {
-		uint32_t c;
 		if (mem_is_null(&argv[i]))
-			c = 0;
+			buf[i] = 0;
 		else if (!mem_is_uint(&argv[i]) || argv[i].u.u > 0x10ffff)
-			c = 0xfffd;
+			buf[i] = 0xfffd;
 		else
-			c = argv[i].u.u;
-		if (c < 0x80) {
-			*ptr++ = c & 0xFF;
-		} else if (c < 0x0800) {
-			*ptr++ = 0xC0 + ((c >> 6) & 0x1F);
-			*ptr++ = 0x80 + (c & 0x3F);
-		} else if (c < 0x10000) {
-			*ptr++ = 0xE0 + ((c >> 12) & 0x0F);
-			*ptr++ = 0x80 + ((c >> 6) & 0x3F);
-			*ptr++ = 0x80 + (c & 0x3F);
-		} else {
-			*ptr++ = 0xF0 + ((c >> 18) & 0x07);
-			*ptr++ = 0x80 + ((c >> 12) & 0x3F);
-			*ptr++ = 0x80 + ((c >> 6) & 0x3F);
-			*ptr++ = 0x80 + (c & 0x3F);
-		}
+			buf[i] = argv[i].u.u;
+		len += U8_LENGTH(buf[i]);
 	}
-	mem_set_str_allocated(ctx->pOut, str, (char *)ptr - str);
+
+	char *str = sqlDbMallocRawNN(sql_get(), len);
+	if (str == NULL) {
+		ctx->is_aborted = true;
+		return;
+	}
+	int pos = 0;
+	for (int i = 0; i < argc; ++i) {
+		bool is_error = false;
+		U8_APPEND(str, pos, len, buf[i], is_error);
+		assert(!is_error);
+		(void)is_error;
+	}
+	region_truncate(region, svp);
+	assert(pos == len);
+	(void)pos;
+	mem_set_str_allocated(ctx->pOut, str, len);
 }
 
 static const unsigned char *



New patch:

commit 4fa0034165697b694b3c655d92a3661ebf80a027
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Tue Oct 5 13:55:21 2021 +0300

    sql: rework CHAR() function
    
    The CHAR() function now uses the ICU macro to get characters.
    
    Part of #4145

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index afe34f7f0..dee28b852 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -717,6 +717,57 @@ func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv)
 		ctx->is_aborted = true;
 }
 
+/**
+ * Implementation of the CHAR() function.
+ *
+ * This function takes zero or more arguments, each of which is an integer. It
+ * constructs a string where each character of the string is the unicode
+ * character for the corresponding integer argument.
+ *
+ * If an argument is negative or greater than 0x10ffff, the symbol "�" is used.
+ * Symbol '\0' used instead of NULL argument.
+ */
+static void
+func_char(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	if (argc == 0)
+		return mem_set_str_static(ctx->pOut, "", 0);
+	struct region *region = &fiber()->gc;
+	size_t svp = region_used(region);
+	UChar32 *buf = region_alloc(region, argc * sizeof(*buf));
+	if (buf == NULL) {
+		ctx->is_aborted = true;
+		return;
+	}
+	int len = 0;
+	for (int i = 0; i < argc; ++i) {
+		if (mem_is_null(&argv[i]))
+			buf[i] = 0;
+		else if (!mem_is_uint(&argv[i]) || argv[i].u.u > 0x10ffff)
+			buf[i] = 0xfffd;
+		else
+			buf[i] = argv[i].u.u;
+		len += U8_LENGTH(buf[i]);
+	}
+
+	char *str = sqlDbMallocRawNN(sql_get(), len);
+	if (str == NULL) {
+		ctx->is_aborted = true;
+		return;
+	}
+	int pos = 0;
+	for (int i = 0; i < argc; ++i) {
+		bool is_error = false;
+		U8_APPEND(str, pos, len, buf[i], is_error);
+		assert(!is_error);
+		(void)is_error;
+	}
+	region_truncate(region, svp);
+	assert(pos == len);
+	(void)pos;
+	mem_set_str_allocated(ctx->pOut, str, len);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1450,50 +1501,6 @@ unicodeFunc(struct sql_context *context, int argc, struct Mem *argv)
 		sql_result_uint(context, sqlUtf8Read(&z));
 }
 
-/*
- * The char() function takes zero or more arguments, each of which is
- * an integer.  It constructs a string where each character of the string
- * is the unicode character for the corresponding integer argument.
- */
-static void
-charFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	unsigned char *z, *zOut;
-	int i;
-	zOut = z = sql_malloc64(argc * 4 + 1);
-	if (z == NULL) {
-		context->is_aborted = true;
-		return;
-	}
-	for (i = 0; i < argc; i++) {
-		uint64_t x;
-		unsigned c;
-		if (sql_value_type(&argv[i]) == MP_INT)
-			x = 0xfffd;
-		else
-			x = mem_get_uint_unsafe(&argv[i]);
-		if (x > 0x10ffff)
-			x = 0xfffd;
-		c = (unsigned)(x & 0x1fffff);
-		if (c < 0x00080) {
-			*zOut++ = (u8) (c & 0xFF);
-		} else if (c < 0x00800) {
-			*zOut++ = 0xC0 + (u8) ((c >> 6) & 0x1F);
-			*zOut++ = 0x80 + (u8) (c & 0x3F);
-		} else if (c < 0x10000) {
-			*zOut++ = 0xE0 + (u8) ((c >> 12) & 0x0F);
-			*zOut++ = 0x80 + (u8) ((c >> 6) & 0x3F);
-			*zOut++ = 0x80 + (u8) (c & 0x3F);
-		} else {
-			*zOut++ = 0xF0 + (u8) ((c >> 18) & 0x07);
-			*zOut++ = 0x80 + (u8) ((c >> 12) & 0x3F);
-			*zOut++ = 0x80 + (u8) ((c >> 6) & 0x3F);
-			*zOut++ = 0x80 + (u8) (c & 0x3F);
-		}
-	}
-	sql_result_text64(context, (char *)z, zOut - z, sql_free);
-}
-
 /*
  * The hex() function.  Interpret the argument as a blob.  Return
  * a hexadecimal rendering as text.
@@ -1846,7 +1853,7 @@ static struct sql_func_definition definitions[] = {
 	 NULL},
 	{"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", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_STRING, func_char, NULL},
 	{"CHAR_LENGTH", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER,
 	 func_char_length, NULL},
 	{"COALESCE", -1, {FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR, sql_builtin_stub,

Re: [Tarantool-patches] [PATCH v1 02/21] sql: refactor GREATEST() and LEAST() functions

[Mergen Imeev via Tarantool-patches]

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

On Fri, Oct 15, 2021 at 12:42:50AM +0200, Vladislav Shpilevoy wrote:
> Thanks for working on this!
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index dd5e7d785..7886f5f40 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -777,6 +777,29 @@ func_char(struct sql_context *ctx, int argc, struct Mem *argv)
> >  	mem_set_str_allocated(ctx->pOut, str, (char *)ptr - str);
> >  }
> >  
> > +/** Implementation of the GREATEST() and LEAST() functions. */
> > +static void
> > +func_greatest_least(struct sql_context *ctx, int argc, struct Mem *argv)
> > +{
> > +	assert(argc > 1);
> > +	int mask = ctx->func->def->name[0] == 'G' ? -1 : 0;
> > +	assert(ctx->func->def->name[0] == 'G' ||
> > +	       ctx->func->def->name[0] == 'L');
> 
> Why don't you use the flag SQL_FUNC_MAX like it was done before?
> 
This flag, together with SQL_FUNC_MIN, has only two uses. The first is to show
that the aggregate function is a MIN() or MAX() function, and use the feature
described in "Define rule for GROUP BY" discussion:
https://github.com/tarantool/tarantool/discussions/6416

Another use is to determine is the function MIN() or MAX().

Since I am planning to drop that feature, I think it is best to remove these
flags from LEAST()/GREATEST() now.

I removed these flags from the definitions of the LEAST()/GREATEST() functions.

In general, I find it awkward to use flags here, and I think we should drop them
in the future. The only useful flag is SQL_FUNC_AGG.

> > +
> > +	if (mem_is_null(&argv[0]))
> > +		return;
> > +	int best = 0;
> > +	for (int i = 1; i < argc; ++i) {
> > +		if (mem_is_null(&argv[i]))
> > +			return;
> > +		int cmp = mem_cmp_scalar(&argv[best], &argv[i], ctx->coll);
> > +		if ((cmp ^ mask) >= 0)
> > +			best = i;
> > +	}
> > +	if (mem_copy(ctx->pOut, &argv[best]) != 0)
> > +		ctx->is_aborted = true;
> > +}

Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 6bde3759d..7d2f03542 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -768,7 +768,12 @@ func_char(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_str_allocated(ctx->pOut, str, len);
 }
 
-/** Implementation of the GREATEST() and LEAST() functions. */
+/**
+ * Implementation of the GREATEST() and LEAST() functions.
+ *
+ * The GREATEST() function returns the largest of the given arguments.
+ * The LEAST() function returns the smallest of the given arguments.
+ */
 static void
 func_greatest_least(struct sql_context *ctx, int argc, struct Mem *argv)
 {
@@ -1766,13 +1771,11 @@ static struct sql_func_dictionary dictionaries[] = {
 	{"CHAR_LENGTH", 1, 1, 0, true, 0, NULL},
 	{"COALESCE", 2, SQL_MAX_FUNCTION_ARG, SQL_FUNC_COALESCE, true, 0, NULL},
 	{"COUNT", 0, 1, SQL_FUNC_AGG, false, 0, NULL},
-	{"GREATEST", 2, SQL_MAX_FUNCTION_ARG, SQL_FUNC_MAX | SQL_FUNC_NEEDCOLL,
-	 true, 0, NULL},
+	{"GREATEST", 2, SQL_MAX_FUNCTION_ARG, SQL_FUNC_NEEDCOLL, true, 0, NULL},
 	{"GROUP_CONCAT", 1, 2, SQL_FUNC_AGG, false, 0, NULL},
 	{"HEX", 1, 1, 0, true, 0, NULL},
 	{"IFNULL", 2, 2, SQL_FUNC_COALESCE, true, 0, NULL},
-	{"LEAST", 2, SQL_MAX_FUNCTION_ARG, SQL_FUNC_MIN | SQL_FUNC_NEEDCOLL,
-	 true, 0, NULL},
+	{"LEAST", 2, SQL_MAX_FUNCTION_ARG, SQL_FUNC_NEEDCOLL, true, 0, NULL},
 	{"LENGTH", 1, 1, SQL_FUNC_LENGTH, true, 0, NULL},
 	{"LIKE", 2, 3, SQL_FUNC_LIKE | SQL_FUNC_NEEDCOLL, true, 0, NULL},
 	{"LIKELIHOOD", 2, 2, SQL_FUNC_UNLIKELY, true, 0, NULL},

Re: [Tarantool-patches] [PATCH v1 03/21] sql: refactor HEX() function

[Mergen Imeev via Tarantool-patches]

Thank you for the review! My answer and diff below. Also, I added a description
of the function.

On Fri, Oct 15, 2021 at 12:43:14AM +0200, Vladislav Shpilevoy wrote:
> Good job on the patch!
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index 7886f5f40..e5d763be1 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -800,6 +800,39 @@ func_greatest_least(struct sql_context *ctx, int argc, struct Mem *argv)
> >  		ctx->is_aborted = true;
> >  }
> >  
> > +/** Implementation of the HEX() function. */
> > +static const char hexdigits[] = {
> > +	'0', '1', '2', '3', '4', '5', '6', '7',
> > +	'8', '9', 'A', 'B', 'C', 'D', 'E', 'F'
> > +};
> > +
> > +static void
> > +func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
> > +{
> > +	assert(argc == 1);
> > +	(void)argc;
> > +	struct Mem *arg = &argv[0];
> > +	if (mem_is_null(arg))
> > +		return mem_set_null(ctx->pOut);
> 
> Isn't pOut already null? I thought it is created as null.
True, fixed.


Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 96525ed3a..2abf6811d 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -796,7 +796,11 @@ func_greatest_least(struct sql_context *ctx, int argc, struct Mem *argv)
 		ctx->is_aborted = true;
 }
 
-/** Implementation of the HEX() function. */
+/**
+ * Implementation of the HEX() function.
+ *
+ * The HEX() function returns the hexadecimal representation of the argument.
+ */
 static const char hexdigits[] = {
 	'0', '1', '2', '3', '4', '5', '6', '7',
 	'8', '9', 'A', 'B', 'C', 'D', 'E', 'F'
@@ -809,7 +813,7 @@ func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
 	(void)argc;
 	struct Mem *arg = &argv[0];
 	if (mem_is_null(arg))
-		return mem_set_null(ctx->pOut);
+		return;
 
 	assert(mem_is_bin(arg) && arg->n >= 0);
 	if (arg->n == 0)

Re: [Tarantool-patches] [PATCH v1 04/21] sql: refactor LENGTH() function

[Mergen Imeev via Tarantool-patches]

Thank you for the review! My answer below.

On Fri, Oct 15, 2021 at 12:43:33AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 08.10.2021 19:31, imeevma@tarantool.org wrote:
> > Part of #4145
> > ---
> >  src/box/sql/func.c | 55 +++++++++++++---------------------------------
> >  1 file changed, 15 insertions(+), 40 deletions(-)
> > 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index e5d763be1..863dbf1c4 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -833,6 +833,19 @@ func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
> >  	mem_set_str_allocated(ctx->pOut, str, size);
> >  }
> >  
> > +/** Implementation of the OCTET_LENGTH() function. */
> > +static void
> > +func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)
> 
> Why is the function LENGTH defined as 'octet_length' instead of just 'length'?
ANSI defines two functions: CHAR_LENGTH() (with CHARACTER_LENGTH() as the other
name) and OCTET_LENGTH(). The first accepts only STRING values and returns the
length in characters or length in octets (depending on the USING clause). The
second accepts VARBINARY or STRING and returns the length in octets.

We have a LENGTH() function that returns the character length if the argument is
STRING, and the octet length if the argument is VARBINARY.

Since I am planning to introduce the USING clause, I find it better to use the
CHAR_LENGTH() and OCTET_LENGTH() implementations for LENGTH() depending on the
type of the argument. The func_octet_length() function will be used cases such
as this:
SELECT char_length(string_value USING OCTETS);

At the moment, I have no plans to introduce OCTET_LENGTH().

Re: [Tarantool-patches] [PATCH v1 05/21] sql: refactor PRINTF() function

[Mergen Imeev via Tarantool-patches]

Thank you for the review! My answer, diff and new patch below. I also simplified
the code a bit.

On Fri, Oct 15, 2021 at 12:44:08AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index 863dbf1c4..f5040fb6e 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -846,6 +846,40 @@ func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)
> >  	mem_set_uint(ctx->pOut, arg->n);
> >  }
> >  
> > +/** Implementation of the PRINTF() function. */
> > +static void
> > +func_printf(struct sql_context *ctx, int argc, struct Mem *argv)
> > +{
> > +	if (argc < 1 || mem_is_null(&argv[0]))
> > +		return;
> > +	if (argc == 1 || !mem_is_str(&argv[0])) {
> > +		struct Mem *mem = ctx->pOut;
> > +		if (mem_copy(mem, &argv[0]) != 0 || mem_to_str(mem) != 0)
> > +			ctx->is_aborted = true;
> > +		return;
> > +	}
> > +	struct PrintfArguments pargs;
> > +	struct StrAccum acc;
> > +	char *format = argv[0].z;
> > +	struct sql *db = sql_get();
> > +
> > +	pargs.nArg = argc - 1;
> > +	pargs.nUsed = 0;
> > +	pargs.apArg = sqlDbMallocRawNN(db, (argc - 1) * sizeof(*pargs.apArg));
> > +	if (pargs.apArg == NULL) {
> > +		ctx->is_aborted = true;
> > +		return;
> > +	}
> > +	for (int i = 1; i < argc; ++i)
> > +		pargs.apArg[i - 1] = &argv[i];
> > +	sqlStrAccumInit(&acc, db, 0, 0, db->aLimit[SQL_LIMIT_LENGTH]);
> > +	acc.printfFlags = SQL_PRINTF_SQLFUNC;
> > +	sqlXPrintf(&acc, format, &pargs);
> > +	sqlDbFree(db, pargs.apArg);
> > +	if (mem_copy_str(ctx->pOut, sqlStrAccumFinish(&acc), acc.nChar) != 0)
> 
> It leaks now, because sqlStrAccumFinish is not destroyed. Previously it
> was 'moved' into the mem via SQL_DYNAMIC. But now you copy it and the
> original is not freed.
Thanks, fixed.


Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 48d248568..de2bbb20e 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -861,23 +861,14 @@ func_printf(struct sql_context *ctx, int argc, struct Mem *argv)
 	struct PrintfArguments pargs;
 	struct StrAccum acc;
 	char *format = argv[0].z;
-	struct sql *db = sql_get();
-
 	pargs.nArg = argc - 1;
 	pargs.nUsed = 0;
-	pargs.apArg = sqlDbMallocRawNN(db, (argc - 1) * sizeof(*pargs.apArg));
-	if (pargs.apArg == NULL) {
-		ctx->is_aborted = true;
-		return;
-	}
-	for (int i = 1; i < argc; ++i)
-		pargs.apArg[i - 1] = &argv[i];
+	pargs.apArg = argv + 1;
+	struct sql *db = sql_get();
 	sqlStrAccumInit(&acc, db, 0, 0, db->aLimit[SQL_LIMIT_LENGTH]);
 	acc.printfFlags = SQL_PRINTF_SQLFUNC;
 	sqlXPrintf(&acc, format, &pargs);
-	sqlDbFree(db, pargs.apArg);
-	if (mem_copy_str(ctx->pOut, sqlStrAccumFinish(&acc), acc.nChar) != 0)
-		ctx->is_aborted = true;
+	mem_set_str_allocated(ctx->pOut, sqlStrAccumFinish(&acc), acc.nChar);
 }
 
 static const unsigned char *
diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
index b4ab0d0f9..5b61646e3 100644
--- a/src/box/sql/printf.c
+++ b/src/box/sql/printf.c
@@ -144,7 +144,7 @@ getIntArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0;
-	return mem_get_int_unsafe(p->apArg[p->nUsed++]);
+	return mem_get_int_unsafe(&p->apArg[p->nUsed++]);
 }
 
 static double
@@ -152,7 +152,7 @@ getDoubleArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0.0;
-	return mem_get_double_unsafe(p->apArg[p->nUsed++]);
+	return mem_get_double_unsafe(&p->apArg[p->nUsed++]);
 }
 
 static char *
@@ -160,7 +160,7 @@ getTextArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0;
-	struct Mem *mem = p->apArg[p->nUsed++];
+	struct Mem *mem = &p->apArg[p->nUsed++];
 	return (char *)mem_as_str0(mem);
 }
 
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index cfdf71f1f..9361775b1 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2511,7 +2511,8 @@ int sqlIsNaN(double);
 struct PrintfArguments {
 	int nArg;		/* Total number of arguments */
 	int nUsed;		/* Number of arguments used so far */
-	sql_value **apArg;	/* The argument values */
+	/** The argument values. */
+	struct Mem *apArg;
 };
 
 void sqlVXPrintf(StrAccum *, const char *, va_list);


New patch:

commit 8d7ab164e7c69faf1a6f74b75deb769da0a0ee27
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Tue Oct 5 18:25:55 2021 +0300

    sql: refactor PRINTF() function
    
    Part of #4145

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 8eb3400bf..de2bbb20e 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -846,6 +846,31 @@ func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_uint(ctx->pOut, arg->n);
 }
 
+/** Implementation of the PRINTF() function. */
+static void
+func_printf(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	if (argc < 1 || mem_is_null(&argv[0]))
+		return;
+	if (argc == 1 || !mem_is_str(&argv[0])) {
+		struct Mem *mem = ctx->pOut;
+		if (mem_copy(mem, &argv[0]) != 0 || mem_to_str(mem) != 0)
+			ctx->is_aborted = true;
+		return;
+	}
+	struct PrintfArguments pargs;
+	struct StrAccum acc;
+	char *format = argv[0].z;
+	pargs.nArg = argc - 1;
+	pargs.nUsed = 0;
+	pargs.apArg = argv + 1;
+	struct sql *db = sql_get();
+	sqlStrAccumInit(&acc, db, 0, 0, db->aLimit[SQL_LIMIT_LENGTH]);
+	acc.printfFlags = SQL_PRINTF_SQLFUNC;
+	sqlXPrintf(&acc, format, &pargs);
+	mem_set_str_allocated(ctx->pOut, sqlStrAccumFinish(&acc), acc.nChar);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -940,40 +965,6 @@ typeofFunc(struct sql_context *context, int argc, struct Mem *argv)
 	sql_result_text(context, z, -1, SQL_STATIC);
 }
 
-/*
- * Implementation of the printf() function.
- */
-static void
-printfFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	PrintfArguments x;
-	StrAccum str;
-	const char *zFormat;
-	int n;
-	sql *db = sql_context_db_handle(context);
-
-	if (argc >= 1 && (zFormat = mem_as_str0(&argv[0])) != NULL) {
-		x.nArg = argc - 1;
-		x.nUsed = 0;
-		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);
-	}
-}
-
 /*
  * Implementation of the round() function
  */
@@ -1916,8 +1907,7 @@ static struct sql_func_definition definitions[] = {
 	 FIELD_TYPE_INTEGER, func_position_characters, NULL},
 	{"POSITION", 2, {FIELD_TYPE_VARBINARY, FIELD_TYPE_VARBINARY},
 	 FIELD_TYPE_INTEGER, func_position_octets, NULL},
-	{"PRINTF", -1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, printfFunc, 
-	 NULL},
+	{"PRINTF", -1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, func_printf, NULL},
 	{"QUOTE", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, quoteFunc, NULL},
 	{"RANDOM", 0, {}, FIELD_TYPE_INTEGER, randomFunc, NULL},
 	{"RANDOMBLOB", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_VARBINARY,
diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
index b4ab0d0f9..5b61646e3 100644
--- a/src/box/sql/printf.c
+++ b/src/box/sql/printf.c
@@ -144,7 +144,7 @@ getIntArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0;
-	return mem_get_int_unsafe(p->apArg[p->nUsed++]);
+	return mem_get_int_unsafe(&p->apArg[p->nUsed++]);
 }
 
 static double
@@ -152,7 +152,7 @@ getDoubleArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0.0;
-	return mem_get_double_unsafe(p->apArg[p->nUsed++]);
+	return mem_get_double_unsafe(&p->apArg[p->nUsed++]);
 }
 
 static char *
@@ -160,7 +160,7 @@ getTextArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0;
-	struct Mem *mem = p->apArg[p->nUsed++];
+	struct Mem *mem = &p->apArg[p->nUsed++];
 	return (char *)mem_as_str0(mem);
 }
 
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index cfdf71f1f..9361775b1 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2511,7 +2511,8 @@ int sqlIsNaN(double);
 struct PrintfArguments {
 	int nArg;		/* Total number of arguments */
 	int nUsed;		/* Number of arguments used so far */
-	sql_value **apArg;	/* The argument values */
+	/** The argument values. */
+	struct Mem *apArg;
 };
 
 void sqlVXPrintf(StrAccum *, const char *, va_list);

Re: [Tarantool-patches] [PATCH v1 06/21] sql: refactor RANDOM() function

[Mergen Imeev via Tarantool-patches]

Thank you for the review! I added a description to the function.

On Fri, Oct 08, 2021 at 08:31:43PM +0300, Mergen Imeev via Tarantool-patches wrote:
<cut>

Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 33436da26..b612c6b62 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -871,7 +871,11 @@ func_printf(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_str_allocated(ctx->pOut, sqlStrAccumFinish(&acc), acc.nChar);
 }
 
-/** Implementation of the RANDOM() function. */
+/**
+ * Implementation of the RANDOM() function.
+ *
+ * This function returns a random INT64 value.
+ */
 static void
 func_random(struct sql_context *ctx, int argc, struct Mem *argv)
 {

Re: [Tarantool-patches] [PATCH v1 07/21] sql: rework RANDOMBLOB() function

[Mergen Imeev via Tarantool-patches]

Thank you for the review! I added a desription to the function.

On Fri, Oct 08, 2021 at 08:31:44PM +0300, Mergen Imeev via Tarantool-patches wrote:
<cut>

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index d1923e854..b12310cad 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -886,7 +886,12 @@ func_random(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_int(ctx->pOut, r, r < 0);
 }
 
-/** Implementation of the RANDOMBLOB() function. */
+/**
+ * Implementation of the RANDOMBLOB() function.
+ *
+ * This function returns a random VARBINARY value. The size of this value is
+ * specified as an argument of the function.
+ */
 static void
 func_randomblob(struct sql_context *ctx, int argc, struct Mem *argv)
 {

Re: [Tarantool-patches] [PATCH v1 08/21] sql: refactor ZEROBLOB() function

[Mergen Imeev via Tarantool-patches]

Thank you for the review! I added a description for the function.

On Fri, Oct 08, 2021 at 08:31:46PM +0300, Mergen Imeev via Tarantool-patches wrote:
<cut>

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 670bed565..22704a9be 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -913,7 +913,12 @@ func_randomblob(struct sql_context *ctx, int argc, struct Mem *argv)
 	mem_set_bin_allocated(ctx->pOut, res, len);
 }
 
-/** Implementation of the ZEROBLOB() function. */
+/**
+ * Implementation of the ZEROBLOB() function.
+ *
+ * This function returns a zero-filled VARBINARY value. The size of this value
+ * is specified as an argument of the function.
+ */
 static void
 func_zeroblob(struct sql_context *ctx, int argc, struct Mem *argv)
 {

Re: [Tarantool-patches] [PATCH v1 12/21] sql: rework UUID() function

[Mergen Imeev via Tarantool-patches]

Thank you for the review! I added a description to the function.

On Fri, Oct 08, 2021 at 08:31:55PM +0300, Mergen Imeev via Tarantool-patches wrote:
<cut>

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 0d9002ac8..a32dd8fb6 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -984,7 +984,11 @@ func_row_count(struct sql_context *ctx, int argc, struct Mem *argv)
 	return mem_set_uint(ctx->pOut, sql_get()->nChange);
 }
 
-/** Implementation of the UUID() function. */
+/**
+ * Implementation of the UUID() function.
+ *
+ * Returns a randomly generated UUID value.
+ */
 static void
 func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
 {

Re: [Tarantool-patches] [PATCH v1 14/21] sql: refactor UNICODE() function

[Mergen Imeev via Tarantool-patches]

Thank you for the review! My answer, diff and new patch below. Also, I added
description to the function.

On Fri, Oct 15, 2021 at 12:44:37AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index fb7fd772e..5e12ef729 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -1007,6 +1007,19 @@ func_version(struct sql_context *ctx, int argc, struct Mem *argv)
> >  	return mem_set_str0_static(ctx->pOut, (char *)tarantool_version());
> >  }
> >  
> > +/** Implementation of the UNICODE() function. */
> > +static void
> > +func_unicode(struct sql_context *ctx, int argc, struct Mem *argv)
> > +{
> > +	assert(argc == 1);
> > +	(void)argc;
> > +	if (mem_is_null(&argv[0]))
> > +		return;
> > +	assert(mem_is_str(&argv[0]));
> > +	const char *str = tt_cstr(argv[0].z, argv[0].n);
> > +	mem_set_uint(ctx->pOut, sqlUtf8Read((const unsigned char **)&str));
> 
> You can dodge the copying. See utf8_next() in utf8.c:
> 
> 	UChar32 c;
> 	U8_NEXT(str, pos, len, c);
Thanks, fixed.


Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index ebc38751e..6d80559d5 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1016,17 +1016,28 @@ func_version(struct sql_context *ctx, int argc, struct Mem *argv)
 	return mem_set_str0_static(ctx->pOut, (char *)tarantool_version());
 }
 
-/** Implementation of the UNICODE() function. */
+/**
+ * Implementation of the UNICODE() function.
+ *
+ * Return the Unicode code point value for the first character of the input
+ * string.
+ */
 static void
 func_unicode(struct sql_context *ctx, int argc, struct Mem *argv)
 {
 	assert(argc == 1);
 	(void)argc;
-	if (mem_is_null(&argv[0]))
+	struct Mem *arg = &argv[0];
+	if (mem_is_null(arg))
 		return;
-	assert(mem_is_str(&argv[0]));
-	const char *str = tt_cstr(argv[0].z, argv[0].n);
-	mem_set_uint(ctx->pOut, sqlUtf8Read((const unsigned char **)&str));
+	assert(mem_is_str(arg));
+	if (arg->n == 0)
+		return mem_set_uint(ctx->pOut, 0);
+	int pos = 0;
+	UChar32 c;
+	U8_NEXT(arg->z, pos, arg->n, c);
+	(void)pos;
+	mem_set_uint(ctx->pOut, (uint64_t)c);
 }
 
 static const unsigned char *


New patch:

commit 6346c542b8c81814753a1853d7ae347222af0f23
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Oct 7 13:43:38 2021 +0300

    sql: refactor UNICODE() function
    
    Part of #4145

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 3afc8ec7f..6d80559d5 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1016,6 +1016,30 @@ func_version(struct sql_context *ctx, int argc, struct Mem *argv)
 	return mem_set_str0_static(ctx->pOut, (char *)tarantool_version());
 }
 
+/**
+ * Implementation of the UNICODE() function.
+ *
+ * Return the Unicode code point value for the first character of the input
+ * string.
+ */
+static void
+func_unicode(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	assert(argc == 1);
+	(void)argc;
+	struct Mem *arg = &argv[0];
+	if (mem_is_null(arg))
+		return;
+	assert(mem_is_str(arg));
+	if (arg->n == 0)
+		return mem_set_uint(ctx->pOut, 0);
+	int pos = 0;
+	UChar32 c;
+	U8_NEXT(arg->z, pos, arg->n, c);
+	(void)pos;
+	mem_set_uint(ctx->pOut, (uint64_t)c);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1437,19 +1461,6 @@ quoteFunc(struct sql_context *context, int argc, struct Mem *argv)
 	}
 }
 
-/*
- * The unicode() function.  Return the integer unicode code-point value
- * for the first character of the input string.
- */
-static void
-unicodeFunc(struct sql_context *context, int argc, struct Mem *argv)
-{
-	const unsigned char *z = mem_as_ustr(&argv[0]);
-	(void)argc;
-	if (z && z[0])
-		sql_result_uint(context, sqlUtf8Read(&z));
-}
-
 /*
  * The replace() function.  Three arguments are all strings: call
  * them A, B, and C. The result is also a string which is derived
@@ -1883,7 +1894,7 @@ static struct sql_func_definition definitions[] = {
 	 FIELD_TYPE_VARBINARY, func_trim_bin, NULL},
 
 	{"TYPEOF", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_STRING, func_typeof, NULL},
-	{"UNICODE", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, unicodeFunc,
+	{"UNICODE", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, func_unicode,
 	 NULL},
 	{"UNLIKELY", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_BOOLEAN, sql_builtin_stub,
 	 NULL},

Re: [Tarantool-patches] [PATCH v1 16/21] sql: refactor REPLACE() function

[Mergen Imeev via Tarantool-patches]

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

On Fri, Oct 15, 2021 at 12:45:29AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 08.10.2021 19:32, Mergen Imeev via Tarantool-patches wrote:
> > Part of #4145
> > ---
> >  src/box/sql/func.c | 58 ++++++++++++++++------------------------------
> >  1 file changed, 20 insertions(+), 38 deletions(-)
> > 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index a3ca53545..f26d101e9 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -1460,34 +1460,26 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
> >  	int i, j;		/* Loop counters */
> >  
> >  	assert(argc == 3);
> > -	UNUSED_PARAMETER(argc);
> > -	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]);
> > -	if (zPattern == 0) {
> > -		assert(mem_is_null(&argv[1])
> > -		       || sql_context_db_handle(context)->mallocFailed);
> > +	(void)argc;
> > +	if (mem_is_any_null(&argv[0], &argv[1]) || mem_is_null(&argv[2]))
> >  		return;
> > -	}
> > -	nPattern = mem_len_unsafe(&argv[1]);
> > +	assert(mem_is_bytes);
> 
> 1. You forgot to call the function.
> 
Thanks, fixed.

> > +	zStr = (const unsigned char *)argv[0].z;
> > +	nStr = argv[0].n;
> > +	zPattern = (const unsigned char *)argv[1].z;
> > +	nPattern = argv[1].n;
> >  	if (nPattern == 0) {
> > -		assert(!mem_is_null(&argv[1]));
> > -		sql_result_value(context, &argv[0]);
> > +		if (mem_copy(context->pOut, &argv[0]) != 0)
> > +			context->is_aborted = true;
> >  		return;
> > @@ -1497,22 +1489,12 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
> >  			zOut[j++] = zStr[i];
> >  		} else {
> >  			u8 *zOld;
> > -			sql *db = sql_context_db_handle(context);
> >  			nOut += nRep - nPattern;
> > -			testcase(nOut - 1 == db->aLimit[SQL_LIMIT_LENGTH]);
> > -			testcase(nOut - 2 == db->aLimit[SQL_LIMIT_LENGTH]);
> > -			if (nOut - 1 > db->aLimit[SQL_LIMIT_LENGTH]) {
> 
> 2. Why did you drop length limit assertions and checks?
> 
These assertions do nothing, since testcase macro defined this way:
#define testcase(X)

I don't think we should check the length of the STRING and VARBINARY values in
functions, since the result is checked in the VDBE.

> > -				diag_set(ClientError, ER_SQL_EXECUTE, "string "\
> > -					 "or binary string is too big");
> > -				context->is_aborted = true;
> > -				sql_free(zOut);
> > -				return;
> > -			}

Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 91ff741df..f7024a9b9 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1485,7 +1485,8 @@ replaceFunc(struct sql_context *context, int argc, struct Mem *argv)
 	(void)argc;
 	if (mem_is_any_null(&argv[0], &argv[1]) || mem_is_null(&argv[2]))
 		return;
-	assert(mem_is_bytes);
+	assert(mem_is_bytes(&argv[0]) && mem_is_bytes(&argv[1]) &&
+	       mem_is_bytes(&argv[2]));
 	zStr = (const unsigned char *)argv[0].z;
 	nStr = argv[0].n;
 	zPattern = (const unsigned char *)argv[1].z;

Re: [Tarantool-patches] [PATCH v1 18/21] sql: remove unused code

[Mergen Imeev via Tarantool-patches]

Thank you for the review! I removed sql/utf.c module, dropped one unused field
of struct Vdbe and removed unused prototype. Diff below.

On Fri, Oct 08, 2021 at 08:32:05PM +0300, Mergen Imeev via Tarantool-patches wrote:
<cut>

Diff:

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index a6de36c66..e578461ef 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -55,7 +55,6 @@ set(sql_sources
     sql/tokenize.c
     sql/treeview.c
     sql/trigger.c
-    sql/utf.c
     sql/update.c
     sql/util.c
     sql/vdbe.c
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 294441e5c..9533833f2 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -823,13 +823,6 @@ mem_len_unsafe(const struct Mem *mem)
 	return len;
 }
 
-/**
- * Return address of memory allocated for accumulation structure of the
- * aggregate function.
- */
-int
-mem_get_agg(const struct Mem *mem, void **accum);
-
 /**
  * Simple type to str convertor. It is used to simplify
  * error reporting.
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2bbf281ee..22a4aa5cd 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3652,26 +3652,6 @@ void sqlDetach(Parse *, Expr *);
 int sqlAtoF(const char *z, double *, int);
 int sqlGetInt32(const char *, int *);
 
-/**
- * Return number of symbols in the given string.
- *
- * Number of symbols != byte size of string because some symbols
- * are encoded with more than one byte. Also note that all
- * symbols from 'str' to 'str + byte_len' would be counted,
- * even if there is a '\0' somewhere between them.
- *
- * This function is implemented to be fast and indifferent to
- * correctness of string being processed. If input string has
- * even one invalid utf-8 sequence, then the resulting length
- * could be arbitary in these boundaries (0 < len < byte_len).
- * @param str String to be counted.
- * @param byte_len Byte length of given string.
- * @return number of symbols in the given string.
- */
-int
-sql_utf8_char_count(const unsigned char *str, int byte_len);
-
-u32 sqlUtf8Read(const u8 **);
 LogEst sqlLogEst(u64);
 LogEst sqlLogEstAdd(LogEst, LogEst);
 u64 sqlLogEstToInt(LogEst);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 7c983fea5..fbd159126 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -805,11 +805,8 @@ sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
 
 		if (!parser->is_aborted)
 			parser->is_aborted = pSubParse->is_aborted;
-		if (db->mallocFailed == 0) {
-			pProgram->aOp =
-			    sqlVdbeTakeOpArray(v, &pProgram->nOp,
-						   &pTop->nMaxArg);
-		}
+		if (db->mallocFailed == 0)
+			pProgram->aOp = sqlVdbeTakeOpArray(v, &pProgram->nOp);
 		pProgram->nMem = pSubParse->nMem;
 		pProgram->nCsr = pSubParse->nTab;
 		pProgram->token = (void *)trigger;
diff --git a/src/box/sql/utf.c b/src/box/sql/utf.c
deleted file mode 100644
index 2d9a12806..000000000
--- a/src/box/sql/utf.c
+++ /dev/null
@@ -1,95 +0,0 @@
-/*
- * Copyright 2010-2017, Tarantool AUTHORS, please see AUTHORS file.
- *
- * Redistribution and use in source and binary forms, with or
- * without modification, are permitted provided that the following
- * conditions are met:
- *
- * 1. Redistributions of source code must retain the above
- *    copyright notice, this list of conditions and the
- *    following disclaimer.
- *
- * 2. Redistributions in binary form must reproduce the above
- *    copyright notice, this list of conditions and the following
- *    disclaimer in the documentation and/or other materials
- *    provided with the distribution.
- *
- * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
- * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
- * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
- * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
- * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
- * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
- * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
- * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
- */
-
-/*
- * This file contains routines used to translate between UTF-8.
- *
- * Notes on UTF-8:
- *
- *   Byte-0    Byte-1    Byte-2    Byte-3    Value
- *  0xxxxxxx                                 00000000 00000000 0xxxxxxx
- *  110yyyyy  10xxxxxx                       00000000 00000yyy yyxxxxxx
- *  1110zzzz  10yyyyyy  10xxxxxx             00000000 zzzzyyyy yyxxxxxx
- *  11110uuu  10uuzzzz  10yyyyyy  10xxxxxx   000uuuuu zzzzyyyy yyxxxxxx
- *
- *
- */
-#include "sqlInt.h"
-
-/*
- * This lookup table is used to help decode the first byte of
- * a multi-byte UTF8 character.
- */
-static const unsigned char sqlUtf8Trans1[] = {
-	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
-	0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
-	0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17,
-	0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f,
-	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
-	0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
-	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
-	0x00, 0x01, 0x02, 0x03, 0x00, 0x01, 0x00, 0x00,
-};
-
-u32
-sqlUtf8Read(const unsigned char **pz	/* Pointer to string from which to read char */
-    )
-{
-	unsigned int c;
-
-	/* Same as READ_UTF8() above but without the zTerm parameter.
-	 * For this routine, we assume the UTF8 string is always zero-terminated.
-	 */
-	c = *((*pz)++);
-	if (c >= 0xc0) {
-		c = sqlUtf8Trans1[c - 0xc0];
-		while ((*(*pz) & 0xc0) == 0x80) {
-			c = (c << 6) + (0x3f & *((*pz)++));
-		}
-		if (c < 0x80
-		    || (c & 0xFFFFF800) == 0xD800
-		    || (c & 0xFFFFFFFE) == 0xFFFE) {
-			c = 0xFFFD;
-		}
-	}
-	return c;
-}
-
-int
-sql_utf8_char_count(const unsigned char *str, int byte_len)
-{
-	int symbol_count = 0;
-	for (int i = 0; i < byte_len;) {
-		SQL_UTF8_FWD_1(str, i, byte_len);
-		symbol_count++;
-	}
-	return symbol_count;
-}
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index e40a1a0b3..e482926b2 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -260,7 +260,10 @@ void sqlVdbeCountChanges(Vdbe *);
 sql *sqlVdbeDb(Vdbe *);
 void sqlVdbeSetSql(Vdbe *, const char *z, int n);
 void sqlVdbeSwap(Vdbe *, Vdbe *);
-VdbeOp *sqlVdbeTakeOpArray(Vdbe *, int *, int *);
+
+struct VdbeOp *
+sqlVdbeTakeOpArray(struct Vdbe *p, int *pnOp);
+
 sql_value *sqlVdbeGetBoundValue(Vdbe *, int);
 char *sqlVdbeExpandSql(Vdbe *, const char *);
 
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 8dbba4908..c45cc9301 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -262,7 +262,6 @@ struct Vdbe {
 
 	Op *aOp;		/* Space to hold the virtual machine's program */
 	Mem *aMem;		/* The memory locations */
-	Mem **apArg;		/* Arguments to currently executing user function */
 	/** SQL metadata for DML/DQL queries. */
 	struct sql_column_metadata *metadata;
 	Mem *pResultSet;	/* Pointer to an array of results */
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 3015760e1..f5a84956c 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -447,21 +447,17 @@ sqlVdbeRunOnlyOnce(Vdbe * p)
  * (1) For each jump instruction with a negative P2 value (a label)
  *     resolve the P2 value to an actual address.
  *
- * (2) Compute the maximum number of arguments used by any SQL function
- *     and store that value in *pMaxFuncArgs.
+ * (2) Initialize the p4.xAdvance pointer on opcodes that use it.
  *
- * (3) Initialize the p4.xAdvance pointer on opcodes that use it.
- *
- * (4) Reclaim the memory allocated for storing labels.
+ * (3) Reclaim the memory allocated for storing labels.
  *
  * This routine will only function correctly if the mkopcodeh.sh generator
  * script numbers the opcodes correctly.  Changes to this routine must be
  * coordinated with changes to mkopcodeh.sh.
  */
 static void
-resolveP2Values(Vdbe * p, int *pMaxFuncArgs)
+resolveP2Values(Vdbe * p)
 {
-	int nMaxArgs = *pMaxFuncArgs;
 	Op *pOp;
 	Parse *pParse = p->pParse;
 	int *aLabel = pParse->aLabel;
@@ -506,7 +502,6 @@ resolveP2Values(Vdbe * p, int *pMaxFuncArgs)
 	sqlDbFree(p->db, pParse->aLabel);
 	pParse->aLabel = 0;
 	pParse->nLabel = 0;
-	*pMaxFuncArgs = nMaxArgs;
 }
 
 /*
@@ -526,17 +521,15 @@ sqlVdbeCurrentAddr(Vdbe * p)
  * vdbeFreeOpArray() function.
  *
  * Before returning, *pnOp is set to the number of entries in the returned
- * array. Also, *pnMaxArg is set to the larger of its current value and
- * the number of entries in the Vdbe.apArg[] array required to execute the
- * returned program.
+ * array.
  */
-VdbeOp *
-sqlVdbeTakeOpArray(Vdbe * p, int *pnOp, int *pnMaxArg)
+struct VdbeOp *
+sqlVdbeTakeOpArray(struct Vdbe *p, int *pnOp)
 {
 	VdbeOp *aOp = p->aOp;
 	assert(aOp && !p->db->mallocFailed);
 
-	resolveP2Values(p, pnMaxArg);
+	resolveP2Values(p);
 	*pnOp = p->nOp;
 	p->aOp = 0;
 	return aOp;
@@ -1485,7 +1478,6 @@ sqlVdbeMakeReady(Vdbe * p,	/* The VDBE */
 	int nVar;		/* Number of parameters */
 	int nMem;		/* Number of VM memory registers */
 	int nCursor;		/* Number of cursors required */
-	int nArg;		/* Number of arguments in subprograms */
 	int n;			/* Loop counter */
 	struct ReusableSpace x;	/* Reusable bulk memory */
 
@@ -1499,7 +1491,6 @@ sqlVdbeMakeReady(Vdbe * p,	/* The VDBE */
 	nVar = pParse->nVar;
 	nMem = pParse->nMem;
 	nCursor = pParse->nTab;
-	nArg = pParse->nMaxArg;
 
 	/* Each cursor uses a memory cell.  The first cursor (cursor 0) can
 	 * use aMem[0] which is not otherwise used by the VDBE program.  Allocate
@@ -1521,7 +1512,7 @@ sqlVdbeMakeReady(Vdbe * p,	/* The VDBE */
 	assert(x.nFree >= 0);
 	assert(EIGHT_BYTE_ALIGNMENT(&x.pSpace[x.nFree]));
 
-	resolveP2Values(p, &nArg);
+	resolveP2Values(p);
 	if (pParse->explain && nMem < 10) {
 		nMem = 10;
 	}
@@ -1541,7 +1532,6 @@ sqlVdbeMakeReady(Vdbe * p,	/* The VDBE */
 		x.nNeeded = 0;
 		p->aMem = allocSpace(&x, p->aMem, nMem * sizeof(Mem));
 		p->aVar = allocSpace(&x, p->aVar, nVar * sizeof(Mem));
-		p->apArg = allocSpace(&x, p->apArg, nArg * sizeof(Mem *));
 		p->apCsr =
 		    allocSpace(&x, p->apCsr, nCursor * sizeof(VdbeCursor *));
 		if (x.nNeeded == 0)

Re: [Tarantool-patches] [PATCH v1 19/21] sql: remove MEM_Dyn flag

[Mergen Imeev via Tarantool-patches]

Thank you for the review! My answer and diff below. There will be a bit more
changes due to chages in the previous patch-sets.

On Fri, Oct 15, 2021 at 12:46:09AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 895003580..a001ac8c5 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -2689,15 +2623,12 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
> >  	if (pMem->type == MEM_TYPE_BIN) {
> >  		int i;
> >  		char c;
> > -		if (f & MEM_Dyn) {
> > -			c = 'z';
> > -			assert((f & (MEM_Static|MEM_Ephem))==0);
> > -		} else if (f & MEM_Static) {
> > +		if (f & MEM_Static) {
> 
> 1. != 0.
> 
Fixed.

> >  			c = 't';
> > -			assert((f & (MEM_Dyn|MEM_Ephem))==0);
> > +			assert((f & MEM_Ephem) == 0);
> >  		} else if (f & MEM_Ephem) {
> >  			c = 'e';
> > -			assert((f & (MEM_Static|MEM_Dyn))==0);
> > +			assert((f & MEM_Static) == 0);
> >  		} else {
> >  			c = 's';
> >  		}
> > @@ -2721,15 +2652,12 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
> >  	} else if (pMem->type == MEM_TYPE_STR) {
> >  		int j, k;
> >  		zBuf[0] = ' ';
> > -		if (f & MEM_Dyn) {
> > -			zBuf[1] = 'z';
> > -			assert((f & (MEM_Static|MEM_Ephem))==0);
> > -		} else if (f & MEM_Static) {
> > +		if (f & MEM_Static) {
> 
> 2. != 0.
> 
Fixed.

> >  			zBuf[1] = 't';
> > -			assert((f & (MEM_Dyn|MEM_Ephem))==0);
> > +			assert((f & MEM_Ephem)==0);
> >  		} else if (f & MEM_Ephem) {
> >  			zBuf[1] = 'e';
> > -			assert((f & (MEM_Static|MEM_Dyn))==0);
> > +			assert((f & MEM_Static)==0);
> 
> 3. Spaces around == here and above.
Fixed.

Diff:

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 4f550f3e4..4150a24f0 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -1854,7 +1854,7 @@ mem_append(struct Mem *mem, const char *value, uint32_t len)
 	if (len == 0)
 		return 0;
 	int new_size = mem->n + len;
-	if (((mem->flags & (MEM_Static | MEM_Dyn | MEM_Ephem)) != 0) ||
+	if (((mem->flags & (MEM_Static | MEM_Ephem)) != 0) ||
 	    mem->szMalloc < new_size) {
 		/*
 		 * Force exponential buffer size growth to avoid having to call
@@ -2628,7 +2628,7 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
 	if (pMem->type == MEM_TYPE_BIN) {
 		int i;
 		char c;
-		if (f & MEM_Static) {
+		if ((f & MEM_Static) != 0) {
 			c = 't';
 			assert((f & MEM_Ephem) == 0);
 		} else if (f & MEM_Ephem) {
@@ -2657,12 +2657,12 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
 	} else if (pMem->type == MEM_TYPE_STR) {
 		int j, k;
 		zBuf[0] = ' ';
-		if (f & MEM_Static) {
+		if ((f & MEM_Static) != 0) {
 			zBuf[1] = 't';
-			assert((f & MEM_Ephem)==0);
+			assert((f & MEM_Ephem) == 0);
 		} else if (f & MEM_Ephem) {
 			zBuf[1] = 'e';
-			assert((f & MEM_Static)==0);
+			assert((f & MEM_Static) == 0);
 		} else {
 			zBuf[1] = 's';
 		}
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 76ba666fd..d2e9cc135 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -225,8 +225,7 @@ mem_is_allocated(const struct Mem *mem)
 static inline bool
 mem_is_trivial(const struct Mem *mem)
 {
-	return mem->szMalloc == 0 && (mem->flags & MEM_Dyn) == 0 &&
-	       mem->type != MEM_TYPE_FRAME;
+	return mem->szMalloc == 0 && mem->type != MEM_TYPE_FRAME;
 }
 
 static inline bool

Re: [Tarantool-patches] [PATCH v1 20/21] sql: remove MEM_Term flag

[Mergen Imeev via Tarantool-patches]

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

On Fri, Oct 15, 2021 at 12:47:33AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
> > index b4ab0d0f9..f261a731c 100644
> > --- a/src/box/sql/printf.c
> > +++ b/src/box/sql/printf.c
> > @@ -160,8 +160,22 @@ getTextArg(PrintfArguments * p)
> >  {
> >  	if (p->nArg <= p->nUsed)
> >  		return 0;
> > -	struct Mem *mem = p->apArg[p->nUsed++];
> > -	return (char *)mem_as_str0(mem);
> > +	struct Mem mem;
> > +	mem_create(&mem);
> > +	mem_copy(&mem, p->apArg[p->nUsed++]);
> 
> 1. mem_copy_as_ephemeral() would be enough. But would be even
> better to have mem_snprintf() which would work like snprintf -
> save string representation of mem into a provided buffer and
> return how many bytes written / need to be written. This though
> could be postponed indefinitely, I don't mind.
> 
I wrote mem_snprintf (), but decided to introduce it a little later, as there
are a couple of questions that need to be answered.

> > +	if (mem_to_str(&mem) != 0) {
> > +		mem_destroy(&mem);
> > +		return NULL;
> > +	}
> > +	char *str = region_alloc(&fiber()->gc, mem.n + 1);
> 
> 2. printf is called for each row. Which means it will leak the
> region. You can either do the snprintf-thing described above to
> write directly into the needed buffer, or at least cleanup the
> region using a save point in the end of sqlVXPrintf().
> 
I edited this code so that now the line will be freed correctly.

> > diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
> > index 6849f13ec..ad064d500 100644
> > --- a/src/box/sql/whereexpr.c
> > +++ b/src/box/sql/whereexpr.c
> > @@ -307,12 +307,18 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
> >  
> >  	op = pRight->op;
> >  	if (op == TK_VARIABLE) {
> > -		Vdbe *pReprepare = pParse->pReprepare;
> > -		int iCol = pRight->iColumn;
> > -		pVal = sqlVdbeGetBoundValue(pReprepare, iCol);
> > -		if (pVal != NULL && mem_is_str(pVal)) {
> > -			if (mem_as_str0(pVal) == NULL)
> > +		var = vdbe_get_bound_value(pParse->pReprepare,
> > +					   pRight->iColumn - 1);
> > +		if (var != NULL && mem_is_str(var)) {
> > +			uint32_t size = var->n + 1;
> > +			char *str = region_alloc(&fiber()->gc, size);
> 
> 3. You leak the fiber's region. AFAIR, it is not cleaned up in the
> end of parsing. Try to use pParse->region.
> 
Thanks, fixed. Also, I added region_truncate() at the end of the function.

> > +			if (str == NULL) {
> > +				diag_set(OutOfMemory, size, "region", "str");
> >  				return -1;
> > +			}
> > +			memcpy(str, var->z, var->n);
> > +			str[var->n] = '\0';
> > +			z = str;
> >  		}


Diff:

diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
index 066aaed9e..2c9fcd467 100644
--- a/src/box/sql/printf.c
+++ b/src/box/sql/printf.c
@@ -167,11 +167,9 @@ getTextArg(PrintfArguments * p)
 		mem_destroy(&mem);
 		return NULL;
 	}
-	char *str = region_alloc(&fiber()->gc, mem.n + 1);
-	if (str == NULL) {
-		diag_set(OutOfMemory, mem.n + 1, "region", "str");
+	char *str = sqlDbMallocRawNN(sql_get(), mem.n + 1);
+	if (str == NULL)
 		return NULL;
-	}
 	memcpy(str, mem.z, mem.n);
 	str[mem.n] = '\0';
 	mem_destroy(&mem);
@@ -691,6 +689,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 			if (bArgList) {
 				bufpt = getTextArg(pArgList);
 				c = bufpt ? bufpt[0] : 0;
+				zExtra = bufpt;
 			} else {
 				c = va_arg(ap, int);
 			}
@@ -711,7 +710,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 		case etDYNSTRING:
 			if (bArgList) {
 				bufpt = getTextArg(pArgList);
-				xtype = etSTRING;
+				xtype = etDYNSTRING;
 			} else {
 				bufpt = va_arg(ap, char *);
 			}
@@ -741,6 +740,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 
 				if (bArgList) {
 					escarg = getTextArg(pArgList);
+					zExtra = escarg;
 				} else {
 					escarg = va_arg(ap, char *);
 				}
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index ad064d500..08a1b4e3a 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -268,7 +268,6 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 	int cnt;
 	/* Database connection. */
 	sql *db = pParse->db;
-	const struct Mem *var = NULL;
 	/* Opcode of pRight. */
 	int op;
 	/* Result code to return. */
@@ -306,12 +305,15 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 		return 0;
 
 	op = pRight->op;
+	struct region *region = &pParse->region;
+	size_t svp = region_used(region);
 	if (op == TK_VARIABLE) {
-		var = vdbe_get_bound_value(pParse->pReprepare,
-					   pRight->iColumn - 1);
+		Vdbe *pReprepare = pParse->pReprepare;
+		int iCol = pRight->iColumn;
+		const struct Mem *var = vdbe_get_bound_value(pReprepare, iCol);
 		if (var != NULL && mem_is_str(var)) {
 			uint32_t size = var->n + 1;
-			char *str = region_alloc(&fiber()->gc, size);
+			char *str = region_alloc(region, size);
 			if (str == NULL) {
 				diag_set(OutOfMemory, size, "region", "str");
 				return -1;
@@ -362,6 +364,7 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 		}
 	}
 
+	region_truncate(region, svp);
 	rc = (z != 0);
 	return rc;
 }


New patch:

commit fa3a78f95d3391c1629e31e2618d956c9ece4d14
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Oct 7 17:40:14 2021 +0300

    sql: remove MEM_Term flag
    
    This patch removes the MEM_Term flag, because after changes in the SQL
    built-in functions, this flag is no longer used.
    
    Needed for #4145

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 4150a24f0..1f5df3332 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -346,21 +346,18 @@ void
 mem_set_str0_ephemeral(struct Mem *mem, char *value)
 {
 	set_str_const(mem, value, strlen(value), MEM_Ephem);
-	mem->flags |= MEM_Term;
 }
 
 void
 mem_set_str0_static(struct Mem *mem, char *value)
 {
 	set_str_const(mem, value, strlen(value), MEM_Static);
-	mem->flags |= MEM_Term;
 }
 
 void
 mem_set_str0_allocated(struct Mem *mem, char *value)
 {
 	set_str_dynamic(mem, value, strlen(value), 0);
-	mem->flags |= MEM_Term;
 }
 
 int
@@ -392,7 +389,6 @@ mem_copy_str0(struct Mem *mem, const char *value)
 	if (mem_copy_str(mem, value, len + 1) != 0)
 		return -1;
 	mem->n = len;
-	mem->flags |= MEM_Term;
 	return 0;
 }
 
@@ -657,24 +653,12 @@ int_to_str0(struct Mem *mem)
 	return mem_copy_str0(mem, str);
 }
 
-static inline int
-str_to_str0(struct Mem *mem)
-{
-	assert(mem->type == MEM_TYPE_STR);
-	if (sqlVdbeMemGrow(mem, mem->n + 1, 1) != 0)
-		return -1;
-	mem->z[mem->n] = '\0';
-	mem->flags |= MEM_Term;
-	mem->flags &= ~MEM_Scalar;
-	return 0;
-}
-
 static inline int
 str_to_bin(struct Mem *mem)
 {
 	assert(mem->type == MEM_TYPE_STR);
 	mem->type = MEM_TYPE_BIN;
-	mem->flags &= ~(MEM_Term | MEM_Scalar);
+	mem->flags &= ~MEM_Scalar;
 	return 0;
 }
 
@@ -725,18 +709,6 @@ bin_to_str(struct Mem *mem)
 	return 0;
 }
 
-static inline int
-bin_to_str0(struct Mem *mem)
-{
-	assert(mem->type == MEM_TYPE_BIN);
-	if (sqlVdbeMemGrow(mem, mem->n + 1, 1) != 0)
-		return -1;
-	mem->z[mem->n] = '\0';
-	mem->type = MEM_TYPE_STR;
-	mem->flags = MEM_Term;
-	return 0;
-}
-
 static inline int
 bin_to_uuid(struct Mem *mem)
 {
@@ -1002,7 +974,7 @@ double_to_str0(struct Mem *mem)
 	sql_snprintf(BUF_SIZE, mem->z, "%!.15g", mem->u.r);
 	mem->n = strlen(mem->z);
 	mem->type = MEM_TYPE_STR;
-	mem->flags = MEM_Term;
+	mem->flags = 0;
 	return 0;
 }
 
@@ -1284,39 +1256,6 @@ mem_to_number(struct Mem *mem)
 	return -1;
 }
 
-int
-mem_to_str0(struct Mem *mem)
-{
-	assert(mem->type < MEM_TYPE_INVALID);
-	switch (mem->type) {
-	case MEM_TYPE_STR:
-		if ((mem->flags & MEM_Term) != 0) {
-			mem->flags &= ~MEM_Scalar;
-			return 0;
-		}
-		return str_to_str0(mem);
-	case MEM_TYPE_INT:
-	case MEM_TYPE_UINT:
-		return int_to_str0(mem);
-	case MEM_TYPE_DOUBLE:
-		return double_to_str0(mem);
-	case MEM_TYPE_BOOL:
-		return bool_to_str0(mem);
-	case MEM_TYPE_BIN:
-		return bin_to_str0(mem);
-	case MEM_TYPE_MAP:
-		return map_to_str0(mem);
-	case MEM_TYPE_ARRAY:
-		return array_to_str0(mem);
-	case MEM_TYPE_UUID:
-		return uuid_to_str0(mem);
-	case MEM_TYPE_DEC:
-		return dec_to_str0(mem);
-	default:
-		return -1;
-	}
-}
-
 int
 mem_to_str(struct Mem *mem)
 {
@@ -1763,15 +1702,6 @@ mem_get_bool(const struct Mem *mem, bool *b)
 	return -1;
 }
 
-int
-mem_get_str0(const struct Mem *mem, const char **s)
-{
-	if (mem->type != MEM_TYPE_STR || (mem->flags & MEM_Term) == 0)
-		return -1;
-	*s = mem->z;
-	return 0;
-}
-
 int
 mem_get_bin(const struct Mem *mem, const char **s)
 {
@@ -1814,7 +1744,7 @@ mem_copy(struct Mem *to, const struct Mem *from)
 	to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc);
 	memcpy(to->zMalloc, to->z, to->n);
 	to->z = to->zMalloc;
-	to->flags &= MEM_Term;
+	to->flags = 0;
 	return 0;
 }
 
@@ -1831,7 +1761,7 @@ mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from)
 		return;
 	if ((to->flags & (MEM_Static | MEM_Ephem)) != 0)
 		return;
-	to->flags &= MEM_Term;
+	to->flags = 0;
 	to->flags |= MEM_Ephem;
 	return;
 }
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index d2e9cc135..1ef8a945b 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -98,13 +98,6 @@ struct Mem {
 /** MEM is of SCALAR meta-type. */
 #define MEM_Scalar    0x0002
 #define MEM_Cleared   0x0200	/* NULL set by OP_Null, not from data */
-
-/* Whenever Mem contains a valid string or blob representation, one of
- * the following flags must be set to determine the memory management
- * policy for Mem.z.  The MEM_Term flag tells us whether or not the
- * string is \000 or \u0000 terminated
- */
-#define MEM_Term      0x0400	/* String rep is nul terminated */
 #define MEM_Static    0x1000	/* Mem.z points to a static string */
 #define MEM_Ephem     0x2000	/* Mem.z points to an ephemeral string */
 
@@ -610,14 +603,6 @@ mem_to_number(struct Mem *mem);
 int
 mem_to_str(struct Mem *mem);
 
-/**
- * Convert the given MEM to STRING. This function and the function above define
- * the rules that are used to convert values of all other types to STRING. In
- * this function, the string received after convertion is NULL-terminated.
- */
-int
-mem_to_str0(struct Mem *mem);
-
 /** Convert the given MEM to given type according to explicit cast rules. */
 int
 mem_cast_explicit(struct Mem *mem, enum field_type type);
@@ -722,27 +707,6 @@ mem_get_bool_unsafe(const struct Mem *mem)
 	return b;
 }
 
-/**
- * Return value for MEM of STRING type if MEM contains a NULL-terminated string.
- * Otherwise convert value of the MEM to NULL-terminated string if possible and
- * return converted value. Original MEM is not changed.
- */
-int
-mem_get_str0(const struct Mem *mem, const char **s);
-
-/**
- * Return value for MEM of STRING type if MEM contains NULL-terminated string.
- * Otherwise convert MEM to MEM of string type that contains NULL-terminated
- * string and return its value. Return NULL if conversion is impossible.
- */
-static inline const char *
-mem_as_str0(struct Mem *mem)
-{
-	if (mem_to_str0(mem) != 0)
-		return NULL;
-	return mem->z;
-}
-
 /**
  * Return value for MEM of VARBINARY type. For MEM of all other types convert
  * value of the MEM to VARBINARY if possible and return converted value.
diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
index 5b61646e3..8da7c9878 100644
--- a/src/box/sql/printf.c
+++ b/src/box/sql/printf.c
@@ -160,8 +160,20 @@ getTextArg(PrintfArguments * p)
 {
 	if (p->nArg <= p->nUsed)
 		return 0;
-	struct Mem *mem = &p->apArg[p->nUsed++];
-	return (char *)mem_as_str0(mem);
+	struct Mem mem;
+	mem_create(&mem);
+	mem_copy_as_ephemeral(&mem, &p->apArg[p->nUsed++]);
+	if (mem_to_str(&mem) != 0) {
+		mem_destroy(&mem);
+		return NULL;
+	}
+	char *str = sqlDbMallocRawNN(sql_get(), mem.n + 1);
+	if (str == NULL)
+		return NULL;
+	memcpy(str, mem.z, mem.n);
+	str[mem.n] = '\0';
+	mem_destroy(&mem);
+	return str;
 }
 
 /*
@@ -677,6 +689,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 			if (bArgList) {
 				bufpt = getTextArg(pArgList);
 				c = bufpt ? bufpt[0] : 0;
+				zExtra = bufpt;
 			} else {
 				c = va_arg(ap, int);
 			}
@@ -697,7 +710,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 		case etDYNSTRING:
 			if (bArgList) {
 				bufpt = getTextArg(pArgList);
-				xtype = etSTRING;
+				xtype = etDYNSTRING;
 			} else {
 				bufpt = va_arg(ap, char *);
 			}
@@ -727,6 +740,7 @@ sqlVXPrintf(StrAccum * pAccum,	/* Accumulate results here */
 
 				if (bArgList) {
 					escarg = getTextArg(pArgList);
+					zExtra = escarg;
 				} else {
 					escarg = va_arg(ap, char *);
 				}
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index e482926b2..22e3370a2 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -256,6 +256,9 @@ vdbe_metadata_set_col_autoincrement(struct Vdbe *p, int idx);
 int
 vdbe_metadata_set_col_span(struct Vdbe *p, int idx, const char *span);
 
+const struct Mem *
+vdbe_get_bound_value(struct Vdbe *vdbe, int id);
+
 void sqlVdbeCountChanges(Vdbe *);
 sql *sqlVdbeDb(Vdbe *);
 void sqlVdbeSetSql(Vdbe *, const char *z, int n);
@@ -264,7 +267,6 @@ void sqlVdbeSwap(Vdbe *, Vdbe *);
 struct VdbeOp *
 sqlVdbeTakeOpArray(struct Vdbe *p, int *pnOp);
 
-sql_value *sqlVdbeGetBoundValue(Vdbe *, int);
 char *sqlVdbeExpandSql(Vdbe *, const char *);
 
 /**
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index f5a84956c..5e7a1caff 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2280,31 +2280,12 @@ sqlVdbeDb(Vdbe * v)
 	return v->db;
 }
 
-/*
- * Return a pointer to an sql_value structure containing the value bound
- * parameter iVar of VM v. Except, if the value is an SQL NULL, return
- * 0 instead. Unless it is NULL, apply type to the value before returning it.
- *
- * The returned value must be freed by the caller using sqlValueFree().
- */
-sql_value *
-sqlVdbeGetBoundValue(struct Vdbe *v, int iVar)
-{
-	assert(iVar > 0);
-	if (v) {
-		Mem *pMem = &v->aVar[iVar - 1];
-		if (!mem_is_null(pMem)) {
-			sql_value *pRet = sqlValueNew(v->db);
-			if (pRet == NULL)
-				return NULL;
-			if (mem_copy(pRet, pMem) != 0) {
-				sqlValueFree(pRet);
-				return NULL;
-			}
-			return pRet;
-		}
-	}
-	return 0;
+const struct Mem *
+vdbe_get_bound_value(struct Vdbe *vdbe, int id)
+{
+	if (vdbe == NULL || id < 0 || id >= vdbe->nVar)
+		return NULL;
+	return &vdbe->aVar[id];
 }
 
 void
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 6849f13ec..08a1b4e3a 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -268,7 +268,6 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 	int cnt;
 	/* Database connection. */
 	sql *db = pParse->db;
-	sql_value *pVal = 0;
 	/* Opcode of pRight. */
 	int op;
 	/* Result code to return. */
@@ -306,13 +305,22 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 		return 0;
 
 	op = pRight->op;
+	struct region *region = &pParse->region;
+	size_t svp = region_used(region);
 	if (op == TK_VARIABLE) {
 		Vdbe *pReprepare = pParse->pReprepare;
 		int iCol = pRight->iColumn;
-		pVal = sqlVdbeGetBoundValue(pReprepare, iCol);
-		if (pVal != NULL && mem_is_str(pVal)) {
-			if (mem_as_str0(pVal) == NULL)
+		const struct Mem *var = vdbe_get_bound_value(pReprepare, iCol);
+		if (var != NULL && mem_is_str(var)) {
+			uint32_t size = var->n + 1;
+			char *str = region_alloc(region, size);
+			if (str == NULL) {
+				diag_set(OutOfMemory, size, "region", "str");
 				return -1;
+			}
+			memcpy(str, var->z, var->n);
+			str[var->n] = '\0';
+			z = str;
 		}
 		assert(pRight->op == TK_VARIABLE || pRight->op == TK_REGISTER);
 	} else if (op == TK_STRING) {
@@ -356,8 +364,8 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 		}
 	}
 
+	region_truncate(region, svp);
 	rc = (z != 0);
-	sqlValueFree(pVal);
 	return rc;
 }
 

Re: [Tarantool-patches] [PATCH v1 01/21] sql: refactor CHAR() function

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the fixes!

See 3 comments below.

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index afe34f7f0..dee28b852 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -717,6 +717,57 @@ func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv)
>  		ctx->is_aborted = true;
>  }
>  
> +/**
> + * Implementation of the CHAR() function.
> + *
> + * This function takes zero or more arguments, each of which is an integer. It
> + * constructs a string where each character of the string is the unicode
> + * character for the corresponding integer argument.
> + *
> + * If an argument is negative or greater than 0x10ffff, the symbol "�" is used.
> + * Symbol '\0' used instead of NULL argument.
> + */
> +static void
> +func_char(struct sql_context *ctx, int argc, struct Mem *argv)
> +{
> +	if (argc == 0)
> +		return mem_set_str_static(ctx->pOut, "", 0);
> +	struct region *region = &fiber()->gc;
> +	size_t svp = region_used(region);
> +	UChar32 *buf = region_alloc(region, argc * sizeof(*buf));

1. Would be better to use region_alloc_array(). Otherwise you
risk to get misaligned data.

> +	if (buf == NULL) {
> +		ctx->is_aborted = true;

2. Need to use diag_set() here.

> +		return;
> +	}
> +	int len = 0;
> +	for (int i = 0; i < argc; ++i) {
> +		if (mem_is_null(&argv[i]))
> +			buf[i] = 0;
> +		else if (!mem_is_uint(&argv[i]) || argv[i].u.u > 0x10ffff)
> +			buf[i] = 0xfffd;
> +		else
> +			buf[i] = argv[i].u.u;
> +		len += U8_LENGTH(buf[i]);
> +	}
> +
> +	char *str = sqlDbMallocRawNN(sql_get(), len);
> +	if (str == NULL) {
> +		ctx->is_aborted = true;

3. region leaks here, doesn't it?

Re: [Tarantool-patches] [PATCH v1 04/21] sql: refactor LENGTH() function

[Vladislav Shpilevoy via Tarantool-patches]

>>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
>>> index e5d763be1..863dbf1c4 100644
>>> --- a/src/box/sql/func.c
>>> +++ b/src/box/sql/func.c
>>> @@ -833,6 +833,19 @@ func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
>>>  	mem_set_str_allocated(ctx->pOut, str, size);
>>>  }
>>>  
>>> +/** Implementation of the OCTET_LENGTH() function. */
>>> +static void
>>> +func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)
>>
>> Why is the function LENGTH defined as 'octet_length' instead of just 'length'?
> ANSI defines two functions: CHAR_LENGTH() (with CHARACTER_LENGTH() as the other
> name) and OCTET_LENGTH(). The first accepts only STRING values and returns the
> length in characters or length in octets (depending on the USING clause). The
> second accepts VARBINARY or STRING and returns the length in octets.
> 
> We have a LENGTH() function that returns the character length if the argument is
> STRING, and the octet length if the argument is VARBINARY.
> 
> Since I am planning to introduce the USING clause, I find it better to use the
> CHAR_LENGTH() and OCTET_LENGTH() implementations for LENGTH() depending on the
> type of the argument. The func_octet_length() function will be used cases such
> as this:
> SELECT char_length(string_value USING OCTETS);
> 
> At the moment, I have no plans to introduce OCTET_LENGTH().

Does ANSI define 'USING' or 'OCTET_LENGTH'? Or both?

Re: [Tarantool-patches] [PATCH v1 19/21] sql: remove MEM_Dyn flag

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the fixes!

SQL_TRANSIENT, SQL_DYNAMIC, and SQL_STATIC can all be deleted here.

Re: [Tarantool-patches] [PATCH v1 01/21] sql: refactor CHAR() function

[Mergen Imeev via Tarantool-patches]

Hi! Thank you for the review! My answer and diff below. Also, I fixed the
argument types passed to the U8_* macros as they were throwing an error for
some targents in CI. I believe this was due to the old ICU.

On Sat, Oct 30, 2021 at 01:42:42AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> See 3 comments below.
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index afe34f7f0..dee28b852 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -717,6 +717,57 @@ func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv)
> >  		ctx->is_aborted = true;
> >  }
> >  
> > +/**
> > + * Implementation of the CHAR() function.
> > + *
> > + * This function takes zero or more arguments, each of which is an integer. It
> > + * constructs a string where each character of the string is the unicode
> > + * character for the corresponding integer argument.
> > + *
> > + * If an argument is negative or greater than 0x10ffff, the symbol "�" is used.
> > + * Symbol '\0' used instead of NULL argument.
> > + */
> > +static void
> > +func_char(struct sql_context *ctx, int argc, struct Mem *argv)
> > +{
> > +	if (argc == 0)
> > +		return mem_set_str_static(ctx->pOut, "", 0);
> > +	struct region *region = &fiber()->gc;
> > +	size_t svp = region_used(region);
> > +	UChar32 *buf = region_alloc(region, argc * sizeof(*buf));
> 
> 1. Would be better to use region_alloc_array(). Otherwise you
> risk to get misaligned data.
> 
Thanks, fixed.

> > +	if (buf == NULL) {
> > +		ctx->is_aborted = true;
> 
> 2. Need to use diag_set() here.
> 
Fixed.

> > +		return;
> > +	}
> > +	int len = 0;
> > +	for (int i = 0; i < argc; ++i) {
> > +		if (mem_is_null(&argv[i]))
> > +			buf[i] = 0;
> > +		else if (!mem_is_uint(&argv[i]) || argv[i].u.u > 0x10ffff)
> > +			buf[i] = 0xfffd;
> > +		else
> > +			buf[i] = argv[i].u.u;
> > +		len += U8_LENGTH(buf[i]);
> > +	}
> > +
> > +	char *str = sqlDbMallocRawNN(sql_get(), len);
> > +	if (str == NULL) {
> > +		ctx->is_aborted = true;
> 
> 3. region leaks here, doesn't it?
True, fixed.


Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 71c46924e..0cd8f8f69 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -745,9 +745,11 @@ func_char(struct sql_context *ctx, int argc, struct Mem *argv)
 		return mem_set_str_static(ctx->pOut, "", 0);
 	struct region *region = &fiber()->gc;
 	size_t svp = region_used(region);
-	UChar32 *buf = region_alloc(region, argc * sizeof(*buf));
+	uint32_t size;
+	UChar32 *buf = region_alloc_array(region, typeof(*buf), argc, &size);
 	if (buf == NULL) {
 		ctx->is_aborted = true;
+		diag_set(OutOfMemory, size, "region_alloc_array", "buf");
 		return;
 	}
 	int len = 0;
@@ -763,13 +765,14 @@ func_char(struct sql_context *ctx, int argc, struct Mem *argv)
 
 	char *str = sqlDbMallocRawNN(sql_get(), len);
 	if (str == NULL) {
+		region_truncate(region, svp);
 		ctx->is_aborted = true;
 		return;
 	}
 	int pos = 0;
 	for (int i = 0; i < argc; ++i) {
-		bool is_error = false;
-		U8_APPEND(str, pos, len, buf[i], is_error);
+		UBool is_error = false;
+		U8_APPEND((uint8_t *)str, pos, len, buf[i], is_error);
 		assert(!is_error);
 		(void)is_error;
 	}

Re: [Tarantool-patches] [PATCH v1 04/21] sql: refactor LENGTH() function

[Mergen Imeev via Tarantool-patches]

Thank you for the review! My answer below.

On Sat, Oct 30, 2021 at 01:42:49AM +0200, Vladislav Shpilevoy wrote:
> >>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> >>> index e5d763be1..863dbf1c4 100644
> >>> --- a/src/box/sql/func.c
> >>> +++ b/src/box/sql/func.c
> >>> @@ -833,6 +833,19 @@ func_hex(struct sql_context *ctx, int argc, struct Mem *argv)
> >>>  	mem_set_str_allocated(ctx->pOut, str, size);
> >>>  }
> >>>  
> >>> +/** Implementation of the OCTET_LENGTH() function. */
> >>> +static void
> >>> +func_octet_length(struct sql_context *ctx, int argc, struct Mem *argv)
> >>
> >> Why is the function LENGTH defined as 'octet_length' instead of just 'length'?
> > ANSI defines two functions: CHAR_LENGTH() (with CHARACTER_LENGTH() as the other
> > name) and OCTET_LENGTH(). The first accepts only STRING values and returns the
> > length in characters or length in octets (depending on the USING clause). The
> > second accepts VARBINARY or STRING and returns the length in octets.
> > 
> > We have a LENGTH() function that returns the character length if the argument is
> > STRING, and the octet length if the argument is VARBINARY.
> > 
> > Since I am planning to introduce the USING clause, I find it better to use the
> > CHAR_LENGTH() and OCTET_LENGTH() implementations for LENGTH() depending on the
> > type of the argument. The func_octet_length() function will be used cases such
> > as this:
> > SELECT char_length(string_value USING OCTETS);
> > 
> > At the moment, I have no plans to introduce OCTET_LENGTH().
> 
> Does ANSI define 'USING' or 'OCTET_LENGTH'? Or both?
ANSI defines both. However CHAR_LENGTH(s USING <CHARACTERS or OCTETS>) defined
for STRING argument only, and OCTET_SIZE() defined for both STRING and
VARBINARY.

Re: [Tarantool-patches] [PATCH v1 14/21] sql: refactor UNICODE() function

[Mergen Imeev via Tarantool-patches]

Thank you for the review! I fixed the type of the argument passed to the U8_NEXT
macro. Diff below.

On Mon, Oct 25, 2021 at 11:40:47AM +0300, Mergen Imeev via Tarantool-patches wrote:
<cut>


Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 7e53cc1ce..69d4e7347 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1049,7 +1049,7 @@ func_unicode(struct sql_context *ctx, int argc, struct Mem *argv)
 		return mem_set_uint(ctx->pOut, 0);
 	int pos = 0;
 	UChar32 c;
-	U8_NEXT(arg->z, pos, arg->n, c);
+	U8_NEXT((uint8_t *)arg->z, pos, arg->n, c);
 	(void)pos;
 	mem_set_uint(ctx->pOut, (uint64_t)c);
 }

Re: [Tarantool-patches] [PATCH v1 19/21] sql: remove MEM_Dyn flag

[Mergen Imeev via Tarantool-patches]

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

On Sat, Oct 30, 2021 at 01:43:12AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> SQL_TRANSIENT, SQL_DYNAMIC, and SQL_STATIC can all be deleted here.
True, thanks. Fixed.

Diff:

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 22a4aa5cd..148350d05 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -370,10 +370,6 @@ sql_vsnprintf(int, char *, const char *, va_list);
 #define MATCH_ONE_WILDCARD '_'
 #define MATCH_ALL_WILDCARD '%'
 
-typedef void (*sql_destructor_type) (void *);
-#define SQL_STATIC      ((sql_destructor_type)0)
-#define SQL_TRANSIENT   ((sql_destructor_type)-1)
-
 /**
  * Compile the UTF-8 encoded SQL statement into
  * a statement handle (struct Vdbe).
@@ -873,16 +869,6 @@ typedef u64 uptr;
  */
 #define IsPowerOfTwo(X) (((X)&((X)-1))==0)
 
-/*
- * The following value as a destructor means to use sqlDbFree().
- * The sqlDbFree() routine requires two parameters instead of the
- * one parameter that destructors normally want.  So we have to introduce
- * this magic value that the code knows to handle differently.  Any
- * pointer will work here as long as it is distinct from sql_STATIC
- * and sql_TRANSIENT.
- */
-#define SQL_DYNAMIC   ((sql_destructor_type)sqlMallocSize)
-
 /*
  * The usual case where Writable Static Data (WSD) is supported,
  * the sql_WSD and GLOBAL macros become no-ops and have zero

Re: [Tarantool-patches] [PATCH v1 00/21] Refactor non-standard and non-aggragate functions

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the fixes!

LGTM.

[Tarantool-patches] [PATCH v1 12/21] sql: rework UUID() function

[Mergen Imeev via Tarantool-patches]

This patch refactors UUID() function. Also, UUID(NULL) now returns NULL.

Part of #4145
---
 src/box/sql/func.c         | 56 +++++++++++++++++---------------------
 test/sql-tap/uuid.test.lua | 11 +++++++-
 2 files changed, 35 insertions(+), 32 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 014fd9af2..dcd0157d0 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -998,6 +998,29 @@ func_row_count(struct sql_context *ctx, int argc, struct Mem *argv)
 	return mem_set_uint(ctx->pOut, sql_get()->nChange);
 }
 
+/**
+ * Implementation of the UUID() function.
+ *
+ * Returns a randomly generated UUID value.
+ */
+static void
+func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+	if (argc == 1) {
+		if (mem_is_null(&argv[0]))
+			return;
+		if (!mem_is_uint(&argv[0]) || argv[0].u.u != 4) {
+			diag_set(ClientError, ER_UNSUPPORTED, "Function UUID",
+				 "versions other than 4");
+			ctx->is_aborted = true;
+			return;
+		}
+	}
+	struct tt_uuid uuid;
+	tt_uuid_create(&uuid);
+	mem_set_uuid(ctx->pOut, &uuid);
+}
+
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
 {
@@ -1016,35 +1039,6 @@ mem_as_bin(struct Mem *mem)
 	return s;
 }
 
-static void
-sql_func_uuid(struct sql_context *ctx, int argc, struct Mem *argv)
-{
-	if (argc > 1) {
-		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "UUID",
-			 "one or zero", argc);
-		ctx->is_aborted = true;
-		return;
-	}
-	if (argc == 1) {
-		uint64_t version;
-		if (mem_get_uint(&argv[0], &version) != 0) {
-			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-				 mem_str(&argv[0]), "integer");
-			ctx->is_aborted = true;
-			return;
-		}
-		if (version != 4) {
-			diag_set(ClientError, ER_UNSUPPORTED, "Function UUID",
-				 "versions other than 4");
-			ctx->is_aborted = true;
-			return;
-		}
-	}
-	struct tt_uuid uuid;
-	tt_uuid_create(&uuid);
-	mem_set_uuid(ctx->pOut, &uuid);
-}
-
 /*
  * Allocate nByte bytes of space using sqlMalloc(). If the
  * allocation fails, return NULL. If nByte is larger than the
@@ -1916,8 +1910,8 @@ static struct sql_func_definition definitions[] = {
 	 NULL},
 	{"UPPER", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, func_lower_upper,
 	 NULL},
-	{"UUID", 0, {}, FIELD_TYPE_UUID, sql_func_uuid, NULL},
-	{"UUID", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_UUID, sql_func_uuid, NULL},
+	{"UUID", 0, {}, FIELD_TYPE_UUID, func_uuid, NULL},
+	{"UUID", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_UUID, func_uuid, NULL},
 	{"VERSION", 0, {}, FIELD_TYPE_STRING, sql_func_version, NULL},
 	{"ZEROBLOB", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_VARBINARY,
 	 func_zeroblob, NULL},
diff --git a/test/sql-tap/uuid.test.lua b/test/sql-tap/uuid.test.lua
index d6c53d123..884b1daf9 100755
--- a/test/sql-tap/uuid.test.lua
+++ b/test/sql-tap/uuid.test.lua
@@ -3,7 +3,7 @@ local build_path = os.getenv("BUILDDIR")
 package.cpath = build_path..'/test/sql-tap/?.so;'..build_path..'/test/sql-tap/?.dylib;'..package.cpath
 
 local test = require("sqltester")
-test:plan(146)
+test:plan(147)
 
 local uuid = require("uuid")
 local uuid1 = uuid.fromstr("11111111-1111-1111-1111-111111111111")
@@ -1290,6 +1290,15 @@ test:do_execsql_test(
         true
     })
 
+-- Make sure the uuid(NULL) returns NULL.
+test:do_execsql_test(
+    "uuid-16.7",
+    [[
+        SELECT uuid(NULL) IS NULL;
+    ]], {
+        true
+    })
+
 -- Make sure STRING of wrong length cannot be cast to UUID.
 test:do_catchsql_test(
     "uuid-17.1",
-- 
2.25.1