Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v1 0/2] Introduce syntax for MAP values is SQL
@ 2021-11-18 14:08 Mergen Imeev via Tarantool-patches
  2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names Mergen Imeev via Tarantool-patches
  2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values Mergen Imeev via Tarantool-patches
  0 siblings, 2 replies; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-11-18 14:08 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch-set introduces a new syntax that can be used to create MAP values.

https://github.com/tarantool/tarantool/issues/4763
https://github.com/tarantool/tarantool/tree/imeevma/gh-4763-syntax-for-map

Mergen Imeev (2):
  sql: properly check bind variable names
  sql: introduce syntax for MAP values

 src/box/sql/expr.c          |  79 +++++++++++++++++++++
 src/box/sql/mem.c           |  39 ++++++++++
 src/box/sql/mem.h           |  16 +++++
 src/box/sql/parse.y         |  63 +++++++++++-----
 src/box/sql/sqlInt.h        |  10 +++
 src/box/sql/tokenize.c      |  45 ++++++------
 src/box/sql/vdbe.c          |  20 ++++++
 test/sql-tap/bind.test.lua  |  15 ++++
 test/sql-tap/map.test.lua   | 138 +++++++++++++++++++++++++++++++++++-
 test/sql-tap/misc1.test.lua |   4 +-
 test/sql/iproto.result      |   2 +-
 11 files changed, 389 insertions(+), 42 deletions(-)
 create mode 100755 test/sql-tap/bind.test.lua

-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
  2021-11-18 14:08 [Tarantool-patches] [PATCH v1 0/2] Introduce syntax for MAP values is SQL Mergen Imeev via Tarantool-patches
@ 2021-11-18 14:08 ` Mergen Imeev via Tarantool-patches
  2021-11-20  0:45   ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values Mergen Imeev via Tarantool-patches
  1 sibling, 1 reply; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-11-18 14:08 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

After this patch, variable names will have to follow the rules defined
for identifiers in SQL. This essentially means that the number will no
longer be used as the first character of the bind variable name.

Part of #4763
---
 src/box/sql/expr.c          | 46 +++++++++++++++++++++++++++++++++++++
 src/box/sql/parse.y         | 32 ++++++++++++--------------
 src/box/sql/sqlInt.h        | 10 ++++++++
 src/box/sql/tokenize.c      | 26 ++++++---------------
 test/sql-tap/bind.test.lua  | 15 ++++++++++++
 test/sql-tap/misc1.test.lua |  4 ++--
 test/sql/iproto.result      |  2 +-
 7 files changed, 96 insertions(+), 39 deletions(-)
 create mode 100755 test/sql-tap/bind.test.lua

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index eb169aeb8..74a98c550 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 	}
 }
 
+struct Expr *
+expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
+{
+	assert(spec != 0 && id != NULL && spec->n == 1);
+	if (id->z - spec->z != 1) {
+		diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN, parse->line_count,
+			 spec->z - parse->zTail + 1, spec->n, spec->z);
+		parse->is_aborted = true;
+		return NULL;
+	}
+	if (parse->parse_only) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
+			 parse->line_count, parse->line_pos,
+			 "bindings are not allowed in DDL");
+		parse->is_aborted = true;
+		return NULL;
+	}
+	if (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) {
+		diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
+			 parse->line_count, spec->n, spec->z);
+		parse->is_aborted = true;
+		return NULL;
+	}
+	uint32_t len = spec->n + id->n;
+	struct Expr *expr = sqlDbMallocRawNN(parse->db,
+					     sizeof(*expr) + len + 1);
+	if (expr == NULL) {
+		parse->is_aborted = true;
+		return NULL;
+	}
+	memset(expr, 0, sizeof(*expr));
+	expr->type = FIELD_TYPE_BOOLEAN;
+	expr->op = TK_VARIABLE;
+	expr->flags = EP_Leaf;
+	expr->iAgg = -1;
+	expr->u.zToken = (char *)(expr + 1);
+	expr->u.zToken[0] = spec->z[0];
+	memcpy(expr->u.zToken + 1, id->z, id->n);
+	expr->u.zToken[len] = '\0';
+	assert(SQL_MAX_EXPR_DEPTH > 0);
+	expr->nHeight = 1;
+
+	sqlExprAssignVarNumber(parse, expr, len);
+	return expr;
+}
+
 /*
  * Recursively delete an expression tree.
  */
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 92fb37dd4..06e6244e3 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1087,31 +1087,29 @@ term(A) ::= INTEGER(X). {
   A.zEnd = X.z + X.n;
   if( A.pExpr ) A.pExpr->flags |= EP_Leaf;
 }
-expr(A) ::= VARIABLE(X).     {
-  Token t = X;
+expr(A) ::= VARNUM(X). {
   if (pParse->parse_only) {
-    spanSet(&A, &t, &t);
     diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS, pParse->line_count,
              pParse->line_pos, "bindings are not allowed in DDL");
     pParse->is_aborted = true;
     A.pExpr = NULL;
-  } else if (!(X.z[0]=='#' && sqlIsdigit(X.z[1]))) {
-    u32 n = X.n;
+    A.zEnd = &X.z[X.n];
+  } else {
     spanExpr(&A, pParse, TK_VARIABLE, X);
-    if (A.pExpr->u.zToken[0] == '?' && n > 1) {
-      diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
-      pParse->is_aborted = true;
-    } else {
-      sqlExprAssignVarNumber(pParse, A.pExpr, n);
-    }
-  }else{
-    assert( t.n>=2 );
-    spanSet(&A, &t, &t);
-    diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
-    pParse->is_aborted = true;
-    A.pExpr = NULL;
+    assert(A.pExpr->u.zToken[0] == '?' && X.n == 1);
+    sqlExprAssignVarNumber(pParse, A.pExpr, X.n);
   }
 }
+expr(A) ::= VARIABLE(X) id(Y).     {
+  A.pExpr = expr_variable(pParse, &X, &Y);
+  A.zStart = X.z;
+  A.zEnd = &Y.z[Y.n];
+}
+expr(A) ::= VARIABLE(X) INTEGER(Y).     {
+  A.pExpr = expr_variable(pParse, &X, &Y);
+  A.zStart = X.z;
+  A.zEnd = &Y.z[Y.n];
+}
 expr(A) ::= expr(A) COLLATE id(C). {
   A.pExpr = sqlExprAddCollateToken(pParse, A.pExpr, &C, 1);
   A.zEnd = &C.z[C.n];
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index dcd71e5bd..ab5b098ee 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2617,6 +2617,16 @@ Expr *sqlExprFunction(Parse *, ExprList *, Token *);
 void sqlExprAssignVarNumber(Parse *, Expr *, u32);
 ExprList *sqlExprListAppendVector(Parse *, ExprList *, IdList *, Expr *);
 
+/**
+ * Parse tokens as a name or a position of bound variable.
+ *
+ * @param parse Parse context.
+ * @param spec Special symbol for bound variable.
+ * @param id Name or position number of bound variable.
+ */
+struct Expr *
+expr_variable(struct Parse *parse, struct Token *spec, struct Token *id);
+
 /**
  * Set the sort order for the last element on the given ExprList.
  *
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index f2d5a2df5..8bc519b9d 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -58,8 +58,8 @@
 #define CC_KYWD       1		/* Alphabetics or '_'.  Usable in a keyword */
 #define CC_ID         2		/* unicode characters usable in IDs */
 #define CC_DIGIT      3		/* Digits */
-#define CC_DOLLAR     4		/* '$' */
-#define CC_VARALPHA   5		/* '@', '#', ':'.  Alphabetic SQL variables */
+/** SQL variables: '@', '#', ':', and '$'. */
+#define CC_VARALPHA   5
 #define CC_VARNUM     6		/* '?'.  Numeric SQL variables */
 #define CC_SPACE      7		/* Space characters */
 #define CC_QUOTE      8		/* '\''. String literals */
@@ -90,7 +90,7 @@ static const char sql_ascii_class[] = {
 /*       x0  x1  x2  x3  x4  x5  x6  x7  x8 x9  xa xb  xc xd xe  xf */
 /* 0x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 7, 28, 7, 7, 7, 27, 27,
 /* 1x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27,
-/* 2x */ 7, 15, 9, 5, 4, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
+/* 2x */ 7, 15, 9, 5, 5, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
 /* 3x */ 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 5, 19, 12, 14, 13, 6,
 /* 4x */ 5, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
 /* 5x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 29, 27, 30, 27, 1,
@@ -184,7 +184,7 @@ int
 sql_token(const char *z, int *type, bool *is_reserved)
 {
 	*is_reserved = false;
-	int i, n;
+	int i;
 	char c, delim;
 	/* Switch on the character-class of the first byte
 	 * of the token. See the comment on the CC_ defines
@@ -369,23 +369,11 @@ sql_token(const char *z, int *type, bool *is_reserved)
 		}
 		return i;
 	case CC_VARNUM:
-		*type = TK_VARIABLE;
-		for (i = 1; sqlIsdigit(z[i]); i++) {
-		}
-		return i;
-	case CC_DOLLAR:
+		*type = TK_VARNUM;
+		return 1;
 	case CC_VARALPHA:
-		n = 0;
 		*type = TK_VARIABLE;
-		for (i = 1; (c = z[i]) != 0; i++) {
-			if (IdChar(c))
-				n++;
-			else
-				break;
-		}
-		if (n == 0)
-			*type = TK_ILLEGAL;
-		return i;
+		return 1;
 	case CC_KYWD:
 		for (i = 1; sql_ascii_class[*(unsigned char*)(z+i)] <= CC_KYWD;
 		     i++) {
diff --git a/test/sql-tap/bind.test.lua b/test/sql-tap/bind.test.lua
new file mode 100755
index 000000000..aaf14a44d
--- /dev/null
+++ b/test/sql-tap/bind.test.lua
@@ -0,0 +1,15 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(1)
+
+-- Make sure that bind variable names cannot start with a number.
+test:do_test(
+    "bind-1",
+    function()
+        local res = {pcall(box.execute, [[SELECT @1asd;]], {{['@1asd'] = 123}})}
+        return {tostring(res[3])}
+    end, {
+        "At line 1 at or near position 9: unrecognized token '1asd'"
+    })
+
+test:finish_test()
diff --git a/test/sql-tap/misc1.test.lua b/test/sql-tap/misc1.test.lua
index f207d3e92..204e070d2 100755
--- a/test/sql-tap/misc1.test.lua
+++ b/test/sql-tap/misc1.test.lua
@@ -1052,7 +1052,7 @@ test:do_catchsql_test(
         select''like''like''like#0;
     ]], {
         -- <misc1-21.1>
-        1, [[Syntax error at line 1 near '#0']]
+        1, [[Syntax error at line 1 near '#']]
         -- </misc1-21.1>
     })
 
@@ -1062,7 +1062,7 @@ test:do_catchsql_test(
         VALUES(0,0x0MATCH#0;
     ]], {
         -- <misc1-21.2>
-        1, [[Syntax error at line 1 near '#0']]
+        1, [[Syntax error at line 1 near '#']]
         -- </misc1-21.2>
     })
 
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 6212aa0c0..5a424596e 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -351,7 +351,7 @@ cn:execute('drop table if exists test3')
 --
 cn:execute('select ?1, ?2, ?3', {1, 2, 3})
 ---
-- error: Syntax error at line 1 near '?1'
+- error: Syntax error at line 1 near '1'
 ...
 cn:execute('select $name, $name2', {1, 2})
 ---
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values
  2021-11-18 14:08 [Tarantool-patches] [PATCH v1 0/2] Introduce syntax for MAP values is SQL Mergen Imeev via Tarantool-patches
  2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names Mergen Imeev via Tarantool-patches
@ 2021-11-18 14:08 ` Mergen Imeev via Tarantool-patches
  2021-11-20  0:46   ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 1 reply; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-11-18 14:08 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch introduces a new syntax that allows to create MAP values in
an SQL query.

Part of #4762

@TarantoolBot document
Title: Syntax for MAP in SQL

The syntax for creating document values is available in SQL. You can use
`{`, ':' and `}` to create a MAP value. Only INTEGER, STRING and UUID
values can be keys in MAP values.

Examples:
```
tarantool> box.execute("SELECT {1 : 'a', 'asd' : 1.5, uuid() : true};")
---
- metadata:
  - name: COLUMN_1
    type: map
  rows:
  - [{1: 'a', 91ca4dbb-c6d4-4468-b4a4-ab1e409dd87e: true, 'asd': 1.5}]
...
```

```
tarantool> box.execute("SELECT {'h' : ['abc', 321], 7 : {'b' : 1.5}};")
---
- metadata:
  - name: COLUMN_1
    type: map
  rows:
  - [{7: {'b': 1.5}, 'h': ['abc', 321]}]
...
```
---
 src/box/sql/expr.c        |  33 +++++++++
 src/box/sql/mem.c         |  39 +++++++++++
 src/box/sql/mem.h         |  16 +++++
 src/box/sql/parse.y       |  35 +++++++++-
 src/box/sql/tokenize.c    |  21 +++++-
 src/box/sql/vdbe.c        |  20 ++++++
 test/sql-tap/map.test.lua | 138 +++++++++++++++++++++++++++++++++++++-
 7 files changed, 296 insertions(+), 6 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 74a98c550..789d8906c 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
 	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
 }
 
+static void
+expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
+{
+	struct Vdbe *vdbe = parser->pVdbe;
+	struct ExprList *list = expr->x.pList;
+	if (list == NULL) {
+		sqlVdbeAddOp3(vdbe, OP_Map, 0, reg, 0);
+		return;
+	}
+	int count = list->nExpr;
+	assert(count % 2 == 0);
+	for (int i = 0; i < count / 2; ++i) {
+		struct Expr *expr = list->a[2 * i].pExpr;
+		enum field_type type = sql_expr_type(expr);
+		if (expr->op != TK_VARIABLE && type != FIELD_TYPE_INTEGER &&
+		    type != FIELD_TYPE_UNSIGNED && type != FIELD_TYPE_STRING &&
+		    type != FIELD_TYPE_UUID) {
+			diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Only "
+				 "integer, string and uuid can be keys in map");
+			parser->is_aborted = true;
+			return;
+		}
+	}
+	int values_reg = parser->nMem + 1;
+	parser->nMem += count;
+	sqlExprCodeExprList(parser, list, values_reg, 0, SQL_ECEL_FACTOR);
+	sqlVdbeAddOp3(vdbe, OP_Map, count, reg, values_reg);
+}
+
 /*
  * Erase column-cache entry number i
  */
@@ -3887,6 +3916,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 		expr_code_array(pParse, pExpr, target);
 		break;
 
+	case TK_MAP:
+		expr_code_map(pParse, pExpr, target);
+		return target;
+
 	case TK_LT:
 	case TK_LE:
 	case TK_GT:
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index b598fe5c2..fe7029341 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -3043,6 +3043,45 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
 	return array;
 }
 
+char *
+mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
+	       struct region *region)
+{
+	assert(count % 2 == 0);
+	size_t used = region_used(region);
+	bool is_error = false;
+	struct mpstream stream;
+	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
+		      set_encode_error, &is_error);
+	mpstream_encode_map(&stream, (count + 1) / 2);
+	for (uint32_t i = 0; i < count / 2; ++i) {
+		const struct Mem *key = &mems[2 * i];
+		const struct Mem *value = &mems[2 * i + 1];
+		if (mem_is_metatype(key) ||
+		    (key->type & (MEM_TYPE_UINT | MEM_TYPE_INT | MEM_TYPE_UUID |
+				  MEM_TYPE_STR)) == 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 mem_str(key), "integer, string or uuid");
+			return NULL;
+		}
+		mem_to_mpstream(key, &stream);
+		mem_to_mpstream(value, &stream);
+	}
+	mpstream_flush(&stream);
+	if (is_error) {
+		diag_set(OutOfMemory, stream.pos - stream.buf,
+			 "mpstream_flush", "stream");
+		return NULL;
+	}
+	*size = region_used(region) - used;
+	char *map = region_join(region, *size);
+	if (map == NULL) {
+		diag_set(OutOfMemory, *size, "region_join", "map");
+		return NULL;
+	}
+	return map;
+}
+
 /**
  * Allocate a sequence of initialized vdbe memory registers
  * on region.
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index b0128921f..83bb30ccf 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -861,3 +861,19 @@ mem_to_mpstream(const struct Mem *var, struct mpstream *stream);
 char *
 mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
 		 struct region *region);
+
+/**
+ * Encode array of MEMs as msgpack map on region. Values in even position are
+ * treated as keys in MAP, values in odd position are treated as values in MAP.
+ * number of MEMs should be even.
+ *
+ * @param mems array of MEMs to encode.
+ * @param count number of elements in the array.
+ * @param[out] size Size of encoded msgpack map.
+ * @param region Region to use.
+ * @retval NULL on error, diag message is set.
+ * @retval Pointer to valid msgpack map on success.
+ */
+char *
+mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
+	       struct region *region);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 06e6244e3..db7fef71a 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1100,12 +1100,12 @@ expr(A) ::= VARNUM(X). {
     sqlExprAssignVarNumber(pParse, A.pExpr, X.n);
   }
 }
-expr(A) ::= VARIABLE(X) id(Y).     {
+expr(A) ::= COLON|VARIABLE(X) id(Y).     {
   A.pExpr = expr_variable(pParse, &X, &Y);
   A.zStart = X.z;
   A.zEnd = &Y.z[Y.n];
 }
-expr(A) ::= VARIABLE(X) INTEGER(Y).     {
+expr(A) ::= COLON|VARIABLE(X) INTEGER(Y).     {
   A.pExpr = expr_variable(pParse, &X, &Y);
   A.zStart = X.z;
   A.zEnd = &Y.z[Y.n];
@@ -1140,6 +1140,37 @@ expr(A) ::= LB(X) exprlist(Y) RB(E). {
   spanSet(&A, &X, &E);
 }
 
+expr(A) ::= LCB(X) maplist(Y) RCB(E). {
+  struct sql *db = pParse->db;
+  struct Expr *expr = sql_expr_new_dequoted(db, TK_MAP, NULL);
+  if (expr == NULL) {
+    sql_expr_list_delete(db, Y);
+    pParse->is_aborted = true;
+    return;
+  }
+  expr->x.pList = Y;
+  expr->type = FIELD_TYPE_MAP;
+  sqlExprSetHeightAndFlags(pParse, expr);
+  A.pExpr = expr;
+  spanSet(&A, &X, &E);
+}
+
+maplist(A) ::= nmaplist(A).
+maplist(A) ::= .                                  {A = 0;}
+nmaplist(A) ::= nmaplist(A) COMMA expr(X) COLON expr(Y). {
+  A = sql_expr_list_append(pParse->db, A, X.pExpr);
+  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
+}
+nmaplist(A) ::= expr(X) COLON expr(Y). {
+  A = sql_expr_list_append(pParse->db, NULL, X.pExpr);
+  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
+}
+
+%type maplist {ExprList*}
+%destructor maplist {sql_expr_list_delete(pParse->db, $$);}
+%type nmaplist {ExprList*}
+%destructor nmaplist {sql_expr_list_delete(pParse->db, $$);}
+
 expr(A) ::= TRIM(X) LP trim_operands(Y) RP(E). {
   A.pExpr = sqlExprFunction(pParse, Y, &X);
   spanSet(&A, &X, &E);
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 8bc519b9d..9e85801a3 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -58,7 +58,9 @@
 #define CC_KYWD       1		/* Alphabetics or '_'.  Usable in a keyword */
 #define CC_ID         2		/* unicode characters usable in IDs */
 #define CC_DIGIT      3		/* Digits */
-/** SQL variables: '@', '#', ':', and '$'. */
+/** Character ':'. */
+#define CC_COLON      4
+/** SQL variable special characters: '@', '#', and '$'. */
 #define CC_VARALPHA   5
 #define CC_VARNUM     6		/* '?'.  Numeric SQL variables */
 #define CC_SPACE      7		/* Space characters */
@@ -85,17 +87,21 @@
 #define CC_LINEFEED  28		/* '\n' */
 #define CC_LB        29		/* '[' */
 #define CC_RB        30		/* ']' */
+/** Character '{'. */
+#define CC_LCB       31
+/** Character '}'. */
+#define CC_RCB       32
 
 static const char sql_ascii_class[] = {
 /*       x0  x1  x2  x3  x4  x5  x6  x7  x8 x9  xa xb  xc xd xe  xf */
 /* 0x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 7, 28, 7, 7, 7, 27, 27,
 /* 1x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27,
 /* 2x */ 7, 15, 9, 5, 5, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
-/* 3x */ 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 5, 19, 12, 14, 13, 6,
+/* 3x */ 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 4, 19, 12, 14, 13, 6,
 /* 4x */ 5, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
 /* 5x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 29, 27, 30, 27, 1,
 /* 6x */ 27, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
-/* 7x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 27, 10, 27, 25, 27,
+/* 7x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 31, 10, 32, 25, 27,
 /* 8x */ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
 /* 9x */ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
 /* Ax */ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
@@ -228,6 +234,12 @@ sql_token(const char *z, int *type, bool *is_reserved)
 	case CC_RB:
 		*type = TK_RB;
 		return 1;
+	case CC_LCB:
+		*type = TK_LCB;
+		return 1;
+	case CC_RCB:
+		*type = TK_RCB;
+		return 1;
 	case CC_SEMI:
 		*type = TK_SEMI;
 		return 1;
@@ -371,6 +383,9 @@ sql_token(const char *z, int *type, bool *is_reserved)
 	case CC_VARNUM:
 		*type = TK_VARNUM;
 		return 1;
+	case CC_COLON:
+		*type = TK_COLON;
+		return 1;
 	case CC_VARALPHA:
 		*type = TK_VARIABLE;
 		return 1;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 55e494332..86de3f98a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1438,6 +1438,26 @@ case OP_Array: {
 	break;
 }
 
+/**
+ * Opcode: Map P1 P2 P3 * *
+ * Synopsis: r[P2] = map(P3@P1)
+ *
+ * Construct an MAP value from P1 registers starting at reg(P3).
+ */
+case OP_Map: {
+	pOut = &aMem[pOp->p2];
+
+	uint32_t size;
+	struct region *region = &fiber()->gc;
+	size_t svp = region_used(region);
+	char *val = mem_encode_map(&aMem[pOp->p3], pOp->p1, &size, region);
+	if (val == NULL || mem_copy_map(pOut, val, size) != 0) {
+		region_truncate(region, svp);
+		goto abort_due_to_error;
+	}
+	break;
+}
+
 /* Opcode: Eq P1 P2 P3 P4 P5
  * Synopsis: IF r[P3]==r[P1]
  *
diff --git a/test/sql-tap/map.test.lua b/test/sql-tap/map.test.lua
index 1afbb2b1d..de1e495f3 100755
--- a/test/sql-tap/map.test.lua
+++ b/test/sql-tap/map.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(110)
+test:plan(126)
 
 box.schema.func.create('M1', {
     language = 'Lua',
@@ -982,6 +982,142 @@ test:do_catchsql_test(
         1, "Failed to execute SQL statement: wrong arguments for function ZEROBLOB()"
     })
 
+-- Make sure syntax for MAP values works as intended.
+test:do_execsql_test(
+    "map-13.1",
+    [[
+        SELECT {'a': a, 'g': g, 't': t, 'n': n, 'f': f, 'i': i, 'b': b, 'v': v,
+                's': s, 'd': d, 'u': u} FROM t1 WHERE id = 1;
+    ]], {
+        {t = "1", f = 1, n = 1, v = "1", g = 1, b = true, s = 1,
+         d = require('decimal').new(1), a = {a = 1}, i = 1,
+         u = require('uuid').fromstr('11111111-1111-1111-1111-111111111111')}
+    })
+
+test:do_execsql_test(
+    "map-13.2",
+    [[
+        SELECT {'q': 1, 'w': true, 'e': 1.5e0, 'r': ['asd', x'32'], 't': 123.0};
+    ]], {
+        {w = true, e = 1.5, r = {'asd', '2'}, t = require('decimal').new(123),
+         q = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.3",
+    [[
+        SELECT typeof({1: 1});
+    ]], {
+        "map"
+    })
+
+-- Make sure MAP() accepts only INTEGER, STRING and UUID as keys.
+test:do_execsql_test(
+    "map-13.4",
+    [[
+        SELECT {1: 1};
+    ]], {
+        {[1] = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.5",
+    [[
+        SELECT {-1: 1};
+    ]], {
+        {[-1] = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.6",
+    [[
+        SELECT {'a': 1};
+    ]], {
+        {a = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.6",
+    [[
+        SELECT typeof({UUID(): 1});
+    ]], {
+        "map"
+    })
+
+test:do_catchsql_test(
+    "map-13.7",
+    [[
+        SELECT {1.5e0: 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.8",
+    [[
+        SELECT {1.5: 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.9",
+    [[
+        SELECT {x'33': 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.10",
+    [[
+        SELECT {[1, 2, 3]: 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.11",
+    [[
+        SELECT {{'a': 1}: 1};
+    ]], {
+        1,
+        'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_catchsql_test(
+    "map-13.12",
+    [[
+        SELECT {CAST(1 AS NUMBER): 1};
+    ]], {
+        1, 'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_catchsql_test(
+    "map-13.13",
+    [[
+        SELECT {CAST(1 AS SCALAR): 1};
+    ]], {
+        1, 'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_catchsql_test(
+    "map-13.14",
+    [[
+        SELECT {CAST(1 AS ANY): 1};
+    ]], {
+        1, 'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_test(
+    "map-13.15",
+    function()
+        local res = {pcall(box.execute, [[SELECT {?: 1};]], {1.5})}
+        return {tostring(res[3])}
+    end, {
+        "Type mismatch: can not convert double(1.5) to integer, string or uuid"
+    })
+
 box.execute([[DROP TABLE t1;]])
 box.execute([[DROP TABLE t;]])
 
-- 
2.25.1


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

* Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
  2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names Mergen Imeev via Tarantool-patches
@ 2021-11-20  0:45   ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-25  8:33     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-20  0:45 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Thanks for the patch!

Shouldn't these be the same?

tarantool> box.execute('SELECT $name', {})
---
- null
- Index of binding slots must start from 1
...

tarantool> box.execute('SELECT @name', {})
---
- metadata:
  - name: COLUMN_1
    type: boolean
  rows:
  - [null]
...

See 4 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index eb169aeb8..74a98c550 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
>  	}
>  }
>  
> +struct Expr *
> +expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)

1. You might want to call it expr_new_variable(). Or sql_expr_new_variable().
To be consistent with our naming policy for constructors allocating memory
and for consistency with with sql_expr_new_column(), sql_expr_new(),
sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon().


2. spec and id can be made const.

> +{
> +	assert(spec != 0 && id != NULL && spec->n == 1);
> +	if (id->z - spec->z != 1) {
> +		diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN, parse->line_count,
> +			 spec->z - parse->zTail + 1, spec->n, spec->z);
> +		parse->is_aborted = true;
> +		return NULL;
> +	}
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 92fb37dd4..06e6244e3 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1087,31 +1087,29 @@ term(A) ::= INTEGER(X). {
>    A.zEnd = X.z + X.n;
>    if( A.pExpr ) A.pExpr->flags |= EP_Leaf;
>  }
> -expr(A) ::= VARIABLE(X).     {
> -  Token t = X;
> +expr(A) ::= VARNUM(X). {
>    if (pParse->parse_only) {
> -    spanSet(&A, &t, &t);
>      diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS, pParse->line_count,
>               pParse->line_pos, "bindings are not allowed in DDL");
>      pParse->is_aborted = true;

3. Hm. It looks not so good that this complicated error is now in
2 places and is exactly the same. Maybe we could have a common
function both for positional and named arguments? Or 2 functions,
but they would use one more generic function inside?

For instance, one function could be

	sql_expr_new_var(struct Parse *parse, const struct Token *spec,
			 const struct Token *id);

For positionals it would be called as

	sql_expr_new_var(pParse, &X, NULL);

For named:

	sql_expr_new_var(pParse, &X, &Y); (like now)

Then we could also drop TK_VARIABLE from spanExpr() completely.

>      A.pExpr = NULL;
> -  } else if (!(X.z[0]=='#' && sqlIsdigit(X.z[1]))) {
> -    u32 n = X.n;
> +    A.zEnd = &X.z[X.n];
> +  } else {
>      spanExpr(&A, pParse, TK_VARIABLE, X);
> -    if (A.pExpr->u.zToken[0] == '?' && n > 1) {
> -      diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
> -      pParse->is_aborted = true;
> -    } else {
> -      sqlExprAssignVarNumber(pParse, A.pExpr, n);
> -    }
> -  }else{
> -    assert( t.n>=2 );
> -    spanSet(&A, &t, &t);
> -    diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
> -    pParse->is_aborted = true;
> -    A.pExpr = NULL;
> +    assert(A.pExpr->u.zToken[0] == '?' && X.n == 1);
> +    sqlExprAssignVarNumber(pParse, A.pExpr, X.n);
>    }
>  }
> +expr(A) ::= VARIABLE(X) id(Y).     {
> +  A.pExpr = expr_variable(pParse, &X, &Y);
> +  A.zStart = X.z;
> +  A.zEnd = &Y.z[Y.n];
> +}
> +expr(A) ::= VARIABLE(X) INTEGER(Y).     {
> +  A.pExpr = expr_variable(pParse, &X, &Y);
> +  A.zStart = X.z;
> +  A.zEnd = &Y.z[Y.n];
> +}

4. Lets drop the extra spaces before { symbols.

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

* Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values
  2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values Mergen Imeev via Tarantool-patches
@ 2021-11-20  0:46   ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-25  8:55     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-20  0:46 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Thanks for the patch!

See 7 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 74a98c550..789d8906c 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
>  	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
>  }
>  
> +static void
> +expr_code_map(struct Parse *parser, struct Expr *expr, int reg)

1. I thought the policy was that we name functions, generating VDBE code,
using 'emit' suffix. For instance, `vdbe_emit_map()` or `sql_emit_map()`.
Don't know about prefix though. I see both vdbe_ and sql_ are used.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index b598fe5c2..fe7029341 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -3043,6 +3043,45 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
>  	return array;
>  }
>  
> +char *
> +mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
> +	       struct region *region)
> +{
> +	assert(count % 2 == 0);
> +	size_t used = region_used(region);
> +	bool is_error = false;
> +	struct mpstream stream;
> +	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> +		      set_encode_error, &is_error);
> +	mpstream_encode_map(&stream, (count + 1) / 2);
> +	for (uint32_t i = 0; i < count / 2; ++i) {
> +		const struct Mem *key = &mems[2 * i];
> +		const struct Mem *value = &mems[2 * i + 1];
> +		if (mem_is_metatype(key) ||
> +		    (key->type & (MEM_TYPE_UINT | MEM_TYPE_INT | MEM_TYPE_UUID |
> +				  MEM_TYPE_STR)) == 0) {
> +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +				 mem_str(key), "integer, string or uuid");
> +			return NULL;
> +		}
> +		mem_to_mpstream(key, &stream);
> +		mem_to_mpstream(value, &stream);
> +	}
> +	mpstream_flush(&stream);
> +	if (is_error) {

2. The error could happen in the last moment after some allocations
were made. Better add a truncate here.

> +		diag_set(OutOfMemory, stream.pos - stream.buf,
> +			 "mpstream_flush", "stream");
> +		return NULL;
> +	}
> +	*size = region_used(region) - used;
> +	char *map = region_join(region, *size);
> +	if (map == NULL) {

3. Ditto. And the same in mem_encode_array().

> +		diag_set(OutOfMemory, *size, "region_join", "map");
> +		return NULL;
> +	}
> +	return map;
> +}
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 06e6244e3..db7fef71a 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1140,6 +1140,37 @@ expr(A) ::= LB(X) exprlist(Y) RB(E). {
>    spanSet(&A, &X, &E);
>  }
>  
> +expr(A) ::= LCB(X) maplist(Y) RCB(E). {
> +  struct sql *db = pParse->db;
> +  struct Expr *expr = sql_expr_new_dequoted(db, TK_MAP, NULL);

4. sql_expr_new_anon().

> +  if (expr == NULL) {
> +    sql_expr_list_delete(db, Y);
> +    pParse->is_aborted = true;
> +    return;
> +  }
> +  expr->x.pList = Y;
> +  expr->type = FIELD_TYPE_MAP;
> +  sqlExprSetHeightAndFlags(pParse, expr);
> +  A.pExpr = expr;
> +  spanSet(&A, &X, &E);
> +}
> +
> +maplist(A) ::= nmaplist(A).
> +maplist(A) ::= .                                  {A = 0;}

5. Lets remove these extra spaces between . and the code block.

> +nmaplist(A) ::= nmaplist(A) COMMA expr(X) COLON expr(Y). {
> +  A = sql_expr_list_append(pParse->db, A, X.pExpr);
> +  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
> +}
> +nmaplist(A) ::= expr(X) COLON expr(Y). {
> +  A = sql_expr_list_append(pParse->db, NULL, X.pExpr);
> +  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
> +}
> +
> +%type maplist {ExprList*}
> +%destructor maplist {sql_expr_list_delete(pParse->db, $$);}
> +%type nmaplist {ExprList*}
> +%destructor nmaplist {sql_expr_list_delete(pParse->db, $$);}

6. Could you please add a whitespace between type and '*'?


7. Lets add some more complicated syntax tests:

tarantool> box.execute('SELECT {:name}', {{[':name'] = 1}})
---
- null
- Syntax error at line 1 near '}'
...

tarantool> box.execute('SELECT {:name: 5}', {{[':name'] = 1}})
---
- metadata:
  - name: COLUMN_1
    type: map
  rows:
  - [{1: 5}]
...

tarantool> box.execute('SELECT {5::name}', {{[':name'] = 1}})
---
- metadata:
  - name: COLUMN_1
    type: map
  rows:
  - [{5: 1}]
...

To see if the parse is able to handle both map ':' and variable ':'.

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

* Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
  2021-11-20  0:45   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-25  8:33     ` Mergen Imeev via Tarantool-patches
  2021-11-30 22:02       ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-11-25  8:33 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

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

On Sat, Nov 20, 2021 at 01:45:36AM +0100, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> Shouldn't these be the same?
> 
I don't know why, but we (like SQLIte) prohibit the use of the "#" character in
front of digits, even if the name begins with a digit. In addition, we disallow
the use of the "$" character with names (we can only use this with numbers),
although this is allowed in SQLite. In general, I think that the work with bound
variables needs to be redesigned. I'll create an issue about this.

> tarantool> box.execute('SELECT $name', {})
> ---
> - null
> - Index of binding slots must start from 1
> ...
> 
> tarantool> box.execute('SELECT @name', {})
> ---
> - metadata:
>   - name: COLUMN_1
>     type: boolean
>   rows:
>   - [null]
> ...
> 
> See 4 comments below.
> 
> > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> > index eb169aeb8..74a98c550 100644
> > --- a/src/box/sql/expr.c
> > +++ b/src/box/sql/expr.c
> > @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
> >  	}
> >  }
> >  
> > +struct Expr *
> > +expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
> 
> 1. You might want to call it expr_new_variable(). Or sql_expr_new_variable().
> To be consistent with our naming policy for constructors allocating memory
> and for consistency with with sql_expr_new_column(), sql_expr_new(),
> sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon().
> 
Thank you! I renamed it to expr_new_variable(). I believe we should drop 'sql_'
prefix for functions that only accessible in SQL.

> 
> 2. spec and id can be made const.
> 
Thanks, fixed.

> > +{
> > +	assert(spec != 0 && id != NULL && spec->n == 1);
> > +	if (id->z - spec->z != 1) {
> > +		diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN, parse->line_count,
> > +			 spec->z - parse->zTail + 1, spec->n, spec->z);
> > +		parse->is_aborted = true;
> > +		return NULL;
> > +	}
> > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> > index 92fb37dd4..06e6244e3 100644
> > --- a/src/box/sql/parse.y
> > +++ b/src/box/sql/parse.y
> > @@ -1087,31 +1087,29 @@ term(A) ::= INTEGER(X). {
> >    A.zEnd = X.z + X.n;
> >    if( A.pExpr ) A.pExpr->flags |= EP_Leaf;
> >  }
> > -expr(A) ::= VARIABLE(X).     {
> > -  Token t = X;
> > +expr(A) ::= VARNUM(X). {
> >    if (pParse->parse_only) {
> > -    spanSet(&A, &t, &t);
> >      diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS, pParse->line_count,
> >               pParse->line_pos, "bindings are not allowed in DDL");
> >      pParse->is_aborted = true;
> 
> 3. Hm. It looks not so good that this complicated error is now in
> 2 places and is exactly the same. Maybe we could have a common
> function both for positional and named arguments? Or 2 functions,
> but they would use one more generic function inside?
> 
> For instance, one function could be
> 
> 	sql_expr_new_var(struct Parse *parse, const struct Token *spec,
> 			 const struct Token *id);
> 
> For positionals it would be called as
> 
> 	sql_expr_new_var(pParse, &X, NULL);
> 
> For named:
> 
> 	sql_expr_new_var(pParse, &X, &Y); (like now)
> 
> Then we could also drop TK_VARIABLE from spanExpr() completely.
> 
Thank you for the suggestions! Fixed.

> >      A.pExpr = NULL;
> > -  } else if (!(X.z[0]=='#' && sqlIsdigit(X.z[1]))) {
> > -    u32 n = X.n;
> > +    A.zEnd = &X.z[X.n];
> > +  } else {
> >      spanExpr(&A, pParse, TK_VARIABLE, X);
> > -    if (A.pExpr->u.zToken[0] == '?' && n > 1) {
> > -      diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
> > -      pParse->is_aborted = true;
> > -    } else {
> > -      sqlExprAssignVarNumber(pParse, A.pExpr, n);
> > -    }
> > -  }else{
> > -    assert( t.n>=2 );
> > -    spanSet(&A, &t, &t);
> > -    diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
> > -    pParse->is_aborted = true;
> > -    A.pExpr = NULL;
> > +    assert(A.pExpr->u.zToken[0] == '?' && X.n == 1);
> > +    sqlExprAssignVarNumber(pParse, A.pExpr, X.n);
> >    }
> >  }
> > +expr(A) ::= VARIABLE(X) id(Y).     {
> > +  A.pExpr = expr_variable(pParse, &X, &Y);
> > +  A.zStart = X.z;
> > +  A.zEnd = &Y.z[Y.n];
> > +}
> > +expr(A) ::= VARIABLE(X) INTEGER(Y).     {
> > +  A.pExpr = expr_variable(pParse, &X, &Y);
> > +  A.zStart = X.z;
> > +  A.zEnd = &Y.z[Y.n];
> > +}
> 
> 4. Lets drop the extra spaces before { symbols.
Fixed.


Diff:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 74a98c550..8ff01dd33 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1315,15 +1315,11 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 }
 
 struct Expr *
-expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
+expr_new_variable(struct Parse *parse, const struct Token *spec,
+		  const struct Token *id)
 {
-	assert(spec != 0 && id != NULL && spec->n == 1);
-	if (id->z - spec->z != 1) {
-		diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN, parse->line_count,
-			 spec->z - parse->zTail + 1, spec->n, spec->z);
-		parse->is_aborted = true;
-		return NULL;
-	}
+	assert(spec != 0 && spec->n == 1);
+	uint32_t len = 1;
 	if (parse->parse_only) {
 		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
 			 parse->line_count, parse->line_pos,
@@ -1331,13 +1327,23 @@ expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
 		parse->is_aborted = true;
 		return NULL;
 	}
-	if (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) {
-		diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
-			 parse->line_count, spec->n, spec->z);
-		parse->is_aborted = true;
-		return NULL;
+	if (id != NULL) {
+		assert(spec->z[0] != '?');
+		if (id->z - spec->z != 1) {
+			diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN,
+				 parse->line_count, spec->z - parse->zTail + 1,
+				 spec->n, spec->z);
+			parse->is_aborted = true;
+			return NULL;
+		}
+		if (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) {
+			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
+				 parse->line_count, spec->n, spec->z);
+			parse->is_aborted = true;
+			return NULL;
+		}
+		len += id->n;
 	}
-	uint32_t len = spec->n + id->n;
 	struct Expr *expr = sqlDbMallocRawNN(parse->db,
 					     sizeof(*expr) + len + 1);
 	if (expr == NULL) {
@@ -1351,7 +1357,8 @@ expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
 	expr->iAgg = -1;
 	expr->u.zToken = (char *)(expr + 1);
 	expr->u.zToken[0] = spec->z[0];
-	memcpy(expr->u.zToken + 1, id->z, id->n);
+	if (id != NULL)
+		memcpy(expr->u.zToken + 1, id->z, id->n);
 	expr->u.zToken[len] = '\0';
 	assert(SQL_MAX_EXPR_DEPTH > 0);
 	expr->nHeight = 1;
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index f6d17a2be..15f6223b0 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1002,15 +1002,6 @@ idlist(A) ::= nm(Y). {
       case TK_UNKNOWN:
         p->type = FIELD_TYPE_BOOLEAN;
         break;
-      case TK_VARIABLE:
-        /*
-         * For variables we set BOOLEAN type since
-         * unassigned bindings will be replaced
-         * with NULL automatically, i.e. without
-         * explicit call of sql_bind_*().
-         */
-        p->type = FIELD_TYPE_BOOLEAN;
-        break;
       default:
         p->type = FIELD_TYPE_SCALAR;
         break;
@@ -1019,20 +1010,16 @@ idlist(A) ::= nm(Y). {
       p->flags = EP_Leaf;
       p->iAgg = -1;
       p->u.zToken = (char*)&p[1];
-      if (op != TK_VARIABLE) {
-        int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
-        if (rc > name_sz) {
-          name_sz = rc;
-          p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
-          if (p == NULL)
-            goto tarantool_error;
-          p->u.zToken = (char *) &p[1];
-          if (sql_normalize_name(p->u.zToken, name_sz, t.z, t.n) > name_sz)
-              unreachable();
+      int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
+      if (rc > name_sz) {
+        name_sz = rc;
+        p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
+        if (p == NULL) {
+          sqlDbFree(pParse->db, p);
+          pParse->is_aborted = true;
         }
-      } else {
-        memcpy(p->u.zToken, t.z, t.n);
-        p->u.zToken[t.n] = 0;
+        p->u.zToken = (char *) &p[1];
+        sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
       }
 #if SQL_MAX_EXPR_DEPTH>0
       p->nHeight = 1;
@@ -1042,9 +1029,6 @@ idlist(A) ::= nm(Y). {
     pOut->zStart = t.z;
     pOut->zEnd = &t.z[t.n];
     return;
-tarantool_error:
-    sqlDbFree(pParse->db, p);
-    pParse->is_aborted = true;
   }
 }
 
@@ -1088,27 +1072,16 @@ term(A) ::= INTEGER(X). {
   if( A.pExpr ) A.pExpr->flags |= EP_Leaf;
 }
 expr(A) ::= VARNUM(X). {
-  if (pParse->parse_only) {
-    diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS, pParse->line_count,
-             pParse->line_pos, "bindings are not allowed in DDL");
-    pParse->is_aborted = true;
-    A.pExpr = NULL;
-    A.zEnd = &X.z[X.n];
-  } else {
-    spanExpr(&A, pParse, TK_VARIABLE, X);
-    assert(A.pExpr->u.zToken[0] == '?' && X.n == 1);
-    sqlExprAssignVarNumber(pParse, A.pExpr, X.n);
-  }
+  A.pExpr = expr_new_variable(pParse, &X, NULL);
+  spanSet(&A, &X, &X);
 }
-expr(A) ::= VARIABLE(X) id(Y).     {
-  A.pExpr = expr_variable(pParse, &X, &Y);
-  A.zStart = X.z;
-  A.zEnd = &Y.z[Y.n];
+expr(A) ::= VARIABLE(X) id(Y). {
+  A.pExpr = expr_new_variable(pParse, &X, &Y);
+  spanSet(&A, &X, &Y);
 }
-expr(A) ::= VARIABLE(X) INTEGER(Y).     {
-  A.pExpr = expr_variable(pParse, &X, &Y);
-  A.zStart = X.z;
-  A.zEnd = &Y.z[Y.n];
+expr(A) ::= VARIABLE(X) INTEGER(Y). {
+  A.pExpr = expr_new_variable(pParse, &X, &Y);
+  spanSet(&A, &X, &Y);
 }
 expr(A) ::= expr(A) COLLATE id(C). {
   A.pExpr = sqlExprAddCollateToken(pParse, A.pExpr, &C, 1);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index ab5b098ee..1c469e3fc 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2625,7 +2625,8 @@ ExprList *sqlExprListAppendVector(Parse *, ExprList *, IdList *, Expr *);
  * @param id Name or position number of bound variable.
  */
 struct Expr *
-expr_variable(struct Parse *parse, struct Token *spec, struct Token *id);
+expr_new_variable(struct Parse *parse, const struct Token *spec,
+		  const struct Token *id);
 
 /**
  * Set the sort order for the last element on the given ExprList.


New patch:

commit d971ccc20c6902097aa85aef5592a38c083d4dd7
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Nov 18 11:02:20 2021 +0300

    sql: properly check bind variable names
    
    After this patch, variable names will have to follow the rules defined
    for identifiers in SQL. This essentially means that the number will no
    longer be used as the first character of the bind variable name.
    
    Part of #4763

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index eb169aeb8..8ff01dd33 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1314,6 +1314,59 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 	}
 }
 
+struct Expr *
+expr_new_variable(struct Parse *parse, const struct Token *spec,
+		  const struct Token *id)
+{
+	assert(spec != 0 && spec->n == 1);
+	uint32_t len = 1;
+	if (parse->parse_only) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
+			 parse->line_count, parse->line_pos,
+			 "bindings are not allowed in DDL");
+		parse->is_aborted = true;
+		return NULL;
+	}
+	if (id != NULL) {
+		assert(spec->z[0] != '?');
+		if (id->z - spec->z != 1) {
+			diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN,
+				 parse->line_count, spec->z - parse->zTail + 1,
+				 spec->n, spec->z);
+			parse->is_aborted = true;
+			return NULL;
+		}
+		if (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) {
+			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
+				 parse->line_count, spec->n, spec->z);
+			parse->is_aborted = true;
+			return NULL;
+		}
+		len += id->n;
+	}
+	struct Expr *expr = sqlDbMallocRawNN(parse->db,
+					     sizeof(*expr) + len + 1);
+	if (expr == NULL) {
+		parse->is_aborted = true;
+		return NULL;
+	}
+	memset(expr, 0, sizeof(*expr));
+	expr->type = FIELD_TYPE_BOOLEAN;
+	expr->op = TK_VARIABLE;
+	expr->flags = EP_Leaf;
+	expr->iAgg = -1;
+	expr->u.zToken = (char *)(expr + 1);
+	expr->u.zToken[0] = spec->z[0];
+	if (id != NULL)
+		memcpy(expr->u.zToken + 1, id->z, id->n);
+	expr->u.zToken[len] = '\0';
+	assert(SQL_MAX_EXPR_DEPTH > 0);
+	expr->nHeight = 1;
+
+	sqlExprAssignVarNumber(parse, expr, len);
+	return expr;
+}
+
 /*
  * Recursively delete an expression tree.
  */
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ee319d5ad..15f6223b0 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1002,15 +1002,6 @@ idlist(A) ::= nm(Y). {
       case TK_UNKNOWN:
         p->type = FIELD_TYPE_BOOLEAN;
         break;
-      case TK_VARIABLE:
-        /*
-         * For variables we set BOOLEAN type since
-         * unassigned bindings will be replaced
-         * with NULL automatically, i.e. without
-         * explicit call of sql_bind_*().
-         */
-        p->type = FIELD_TYPE_BOOLEAN;
-        break;
       default:
         p->type = FIELD_TYPE_SCALAR;
         break;
@@ -1019,20 +1010,16 @@ idlist(A) ::= nm(Y). {
       p->flags = EP_Leaf;
       p->iAgg = -1;
       p->u.zToken = (char*)&p[1];
-      if (op != TK_VARIABLE) {
-        int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
-        if (rc > name_sz) {
-          name_sz = rc;
-          p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
-          if (p == NULL)
-            goto tarantool_error;
-          p->u.zToken = (char *) &p[1];
-          if (sql_normalize_name(p->u.zToken, name_sz, t.z, t.n) > name_sz)
-              unreachable();
+      int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
+      if (rc > name_sz) {
+        name_sz = rc;
+        p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
+        if (p == NULL) {
+          sqlDbFree(pParse->db, p);
+          pParse->is_aborted = true;
         }
-      } else {
-        memcpy(p->u.zToken, t.z, t.n);
-        p->u.zToken[t.n] = 0;
+        p->u.zToken = (char *) &p[1];
+        sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
       }
 #if SQL_MAX_EXPR_DEPTH>0
       p->nHeight = 1;
@@ -1042,9 +1029,6 @@ idlist(A) ::= nm(Y). {
     pOut->zStart = t.z;
     pOut->zEnd = &t.z[t.n];
     return;
-tarantool_error:
-    sqlDbFree(pParse->db, p);
-    pParse->is_aborted = true;
   }
 }
 
@@ -1087,30 +1071,17 @@ term(A) ::= INTEGER(X). {
   A.zEnd = X.z + X.n;
   if( A.pExpr ) A.pExpr->flags |= EP_Leaf;
 }
-expr(A) ::= VARIABLE(X).     {
-  Token t = X;
-  if (pParse->parse_only) {
-    spanSet(&A, &t, &t);
-    diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS, pParse->line_count,
-             pParse->line_pos, "bindings are not allowed in DDL");
-    pParse->is_aborted = true;
-    A.pExpr = NULL;
-  } else if (!(X.z[0]=='#' && sqlIsdigit(X.z[1]))) {
-    u32 n = X.n;
-    spanExpr(&A, pParse, TK_VARIABLE, X);
-    if (A.pExpr->u.zToken[0] == '?' && n > 1) {
-      diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
-      pParse->is_aborted = true;
-    } else {
-      sqlExprAssignVarNumber(pParse, A.pExpr, n);
-    }
-  }else{
-    assert( t.n>=2 );
-    spanSet(&A, &t, &t);
-    diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
-    pParse->is_aborted = true;
-    A.pExpr = NULL;
-  }
+expr(A) ::= VARNUM(X). {
+  A.pExpr = expr_new_variable(pParse, &X, NULL);
+  spanSet(&A, &X, &X);
+}
+expr(A) ::= VARIABLE(X) id(Y). {
+  A.pExpr = expr_new_variable(pParse, &X, &Y);
+  spanSet(&A, &X, &Y);
+}
+expr(A) ::= VARIABLE(X) INTEGER(Y). {
+  A.pExpr = expr_new_variable(pParse, &X, &Y);
+  spanSet(&A, &X, &Y);
 }
 expr(A) ::= expr(A) COLLATE id(C). {
   A.pExpr = sqlExprAddCollateToken(pParse, A.pExpr, &C, 1);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index dcd71e5bd..1c469e3fc 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2617,6 +2617,17 @@ Expr *sqlExprFunction(Parse *, ExprList *, Token *);
 void sqlExprAssignVarNumber(Parse *, Expr *, u32);
 ExprList *sqlExprListAppendVector(Parse *, ExprList *, IdList *, Expr *);
 
+/**
+ * Parse tokens as a name or a position of bound variable.
+ *
+ * @param parse Parse context.
+ * @param spec Special symbol for bound variable.
+ * @param id Name or position number of bound variable.
+ */
+struct Expr *
+expr_new_variable(struct Parse *parse, const struct Token *spec,
+		  const struct Token *id);
+
 /**
  * Set the sort order for the last element on the given ExprList.
  *
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index f2d5a2df5..8bc519b9d 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -58,8 +58,8 @@
 #define CC_KYWD       1		/* Alphabetics or '_'.  Usable in a keyword */
 #define CC_ID         2		/* unicode characters usable in IDs */
 #define CC_DIGIT      3		/* Digits */
-#define CC_DOLLAR     4		/* '$' */
-#define CC_VARALPHA   5		/* '@', '#', ':'.  Alphabetic SQL variables */
+/** SQL variables: '@', '#', ':', and '$'. */
+#define CC_VARALPHA   5
 #define CC_VARNUM     6		/* '?'.  Numeric SQL variables */
 #define CC_SPACE      7		/* Space characters */
 #define CC_QUOTE      8		/* '\''. String literals */
@@ -90,7 +90,7 @@ static const char sql_ascii_class[] = {
 /*       x0  x1  x2  x3  x4  x5  x6  x7  x8 x9  xa xb  xc xd xe  xf */
 /* 0x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 7, 28, 7, 7, 7, 27, 27,
 /* 1x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27,
-/* 2x */ 7, 15, 9, 5, 4, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
+/* 2x */ 7, 15, 9, 5, 5, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
 /* 3x */ 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 5, 19, 12, 14, 13, 6,
 /* 4x */ 5, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
 /* 5x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 29, 27, 30, 27, 1,
@@ -184,7 +184,7 @@ int
 sql_token(const char *z, int *type, bool *is_reserved)
 {
 	*is_reserved = false;
-	int i, n;
+	int i;
 	char c, delim;
 	/* Switch on the character-class of the first byte
 	 * of the token. See the comment on the CC_ defines
@@ -369,23 +369,11 @@ sql_token(const char *z, int *type, bool *is_reserved)
 		}
 		return i;
 	case CC_VARNUM:
-		*type = TK_VARIABLE;
-		for (i = 1; sqlIsdigit(z[i]); i++) {
-		}
-		return i;
-	case CC_DOLLAR:
+		*type = TK_VARNUM;
+		return 1;
 	case CC_VARALPHA:
-		n = 0;
 		*type = TK_VARIABLE;
-		for (i = 1; (c = z[i]) != 0; i++) {
-			if (IdChar(c))
-				n++;
-			else
-				break;
-		}
-		if (n == 0)
-			*type = TK_ILLEGAL;
-		return i;
+		return 1;
 	case CC_KYWD:
 		for (i = 1; sql_ascii_class[*(unsigned char*)(z+i)] <= CC_KYWD;
 		     i++) {
diff --git a/test/sql-tap/bind.test.lua b/test/sql-tap/bind.test.lua
new file mode 100755
index 000000000..aaf14a44d
--- /dev/null
+++ b/test/sql-tap/bind.test.lua
@@ -0,0 +1,15 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(1)
+
+-- Make sure that bind variable names cannot start with a number.
+test:do_test(
+    "bind-1",
+    function()
+        local res = {pcall(box.execute, [[SELECT @1asd;]], {{['@1asd'] = 123}})}
+        return {tostring(res[3])}
+    end, {
+        "At line 1 at or near position 9: unrecognized token '1asd'"
+    })
+
+test:finish_test()
diff --git a/test/sql-tap/misc1.test.lua b/test/sql-tap/misc1.test.lua
index f207d3e92..204e070d2 100755
--- a/test/sql-tap/misc1.test.lua
+++ b/test/sql-tap/misc1.test.lua
@@ -1052,7 +1052,7 @@ test:do_catchsql_test(
         select''like''like''like#0;
     ]], {
         -- <misc1-21.1>
-        1, [[Syntax error at line 1 near '#0']]
+        1, [[Syntax error at line 1 near '#']]
         -- </misc1-21.1>
     })
 
@@ -1062,7 +1062,7 @@ test:do_catchsql_test(
         VALUES(0,0x0MATCH#0;
     ]], {
         -- <misc1-21.2>
-        1, [[Syntax error at line 1 near '#0']]
+        1, [[Syntax error at line 1 near '#']]
         -- </misc1-21.2>
     })
 
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 6212aa0c0..5a424596e 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -351,7 +351,7 @@ cn:execute('drop table if exists test3')
 --
 cn:execute('select ?1, ?2, ?3', {1, 2, 3})
 ---
-- error: Syntax error at line 1 near '?1'
+- error: Syntax error at line 1 near '1'
 ...
 cn:execute('select $name, $name2', {1, 2})
 ---

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

* Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values
  2021-11-20  0:46   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-25  8:55     ` Mergen Imeev via Tarantool-patches
  2021-11-30 22:04       ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-11-25  8:55 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Thank you for the review! My answers, diff and new patch below. Also, I added
changelog and tests to show that it is possible to create an empty MAP and a
map with more than 1000 key-value pairs.

On Sat, Nov 20, 2021 at 01:46:57AM +0100, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 7 comments below.
> 
> > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> > index 74a98c550..789d8906c 100644
> > --- a/src/box/sql/expr.c
> > +++ b/src/box/sql/expr.c
> > @@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
> >  	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
> >  }
> >  
> > +static void
> > +expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
> 
> 1. I thought the policy was that we name functions, generating VDBE code,
> using 'emit' suffix. For instance, `vdbe_emit_map()` or `sql_emit_map()`.
> Don't know about prefix though. I see both vdbe_ and sql_ are used.
>
This is usually true, but this function is actually part of sqlExprCodeTarget().
I believe these functions were created to make sqlExprCodeTarget() more
readable. All such functions are named sqlExprCode*(), code*() or
expr_code _*(), for example: sqlExprCodeGetColumn(), codeReal(),
expr_code_int().

Since all these functions are static, I think we should drop "expr_" prefix for
them. Not in this patch, though.

> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index b598fe5c2..fe7029341 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -3043,6 +3043,45 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
> >  	return array;
> >  }
> >  
> > +char *
> > +mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
> > +	       struct region *region)
> > +{
> > +	assert(count % 2 == 0);
> > +	size_t used = region_used(region);
> > +	bool is_error = false;
> > +	struct mpstream stream;
> > +	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> > +		      set_encode_error, &is_error);
> > +	mpstream_encode_map(&stream, (count + 1) / 2);
> > +	for (uint32_t i = 0; i < count / 2; ++i) {
> > +		const struct Mem *key = &mems[2 * i];
> > +		const struct Mem *value = &mems[2 * i + 1];
> > +		if (mem_is_metatype(key) ||
> > +		    (key->type & (MEM_TYPE_UINT | MEM_TYPE_INT | MEM_TYPE_UUID |
> > +				  MEM_TYPE_STR)) == 0) {
> > +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +				 mem_str(key), "integer, string or uuid");
> > +			return NULL;
> > +		}
> > +		mem_to_mpstream(key, &stream);
> > +		mem_to_mpstream(value, &stream);
> > +	}
> > +	mpstream_flush(&stream);
> > +	if (is_error) {
> 
> 2. The error could happen in the last moment after some allocations
> were made. Better add a truncate here.
> 
Fixed.

> > +		diag_set(OutOfMemory, stream.pos - stream.buf,
> > +			 "mpstream_flush", "stream");
> > +		return NULL;
> > +	}
> > +	*size = region_used(region) - used;
> > +	char *map = region_join(region, *size);
> > +	if (map == NULL) {
> 
> 3. Ditto. And the same in mem_encode_array().
> 
Fixed.

> > +		diag_set(OutOfMemory, *size, "region_join", "map");
> > +		return NULL;
> > +	}
> > +	return map;
> > +}
> > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> > index 06e6244e3..db7fef71a 100644
> > --- a/src/box/sql/parse.y
> > +++ b/src/box/sql/parse.y
> > @@ -1140,6 +1140,37 @@ expr(A) ::= LB(X) exprlist(Y) RB(E). {
> >    spanSet(&A, &X, &E);
> >  }
> >  
> > +expr(A) ::= LCB(X) maplist(Y) RCB(E). {
> > +  struct sql *db = pParse->db;
> > +  struct Expr *expr = sql_expr_new_dequoted(db, TK_MAP, NULL);
> 
> 4. sql_expr_new_anon().
> 
Fixed.

> > +  if (expr == NULL) {
> > +    sql_expr_list_delete(db, Y);
> > +    pParse->is_aborted = true;
> > +    return;
> > +  }
> > +  expr->x.pList = Y;
> > +  expr->type = FIELD_TYPE_MAP;
> > +  sqlExprSetHeightAndFlags(pParse, expr);
> > +  A.pExpr = expr;
> > +  spanSet(&A, &X, &E);
> > +}
> > +
> > +maplist(A) ::= nmaplist(A).
> > +maplist(A) ::= .                                  {A = 0;}
> 
> 5. Lets remove these extra spaces between . and the code block.
> 
Fixed.

> > +nmaplist(A) ::= nmaplist(A) COMMA expr(X) COLON expr(Y). {
> > +  A = sql_expr_list_append(pParse->db, A, X.pExpr);
> > +  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
> > +}
> > +nmaplist(A) ::= expr(X) COLON expr(Y). {
> > +  A = sql_expr_list_append(pParse->db, NULL, X.pExpr);
> > +  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
> > +}
> > +
> > +%type maplist {ExprList*}
> > +%destructor maplist {sql_expr_list_delete(pParse->db, $$);}
> > +%type nmaplist {ExprList*}
> > +%destructor nmaplist {sql_expr_list_delete(pParse->db, $$);}
> 
> 6. Could you please add a whitespace between type and '*'?
> 
Fixed.

> 
> 7. Lets add some more complicated syntax tests:
> 
> tarantool> box.execute('SELECT {:name}', {{[':name'] = 1}})
> ---
> - null
> - Syntax error at line 1 near '}'
> ...
> 
> tarantool> box.execute('SELECT {:name: 5}', {{[':name'] = 1}})
> ---
> - metadata:
>   - name: COLUMN_1
>     type: map
>   rows:
>   - [{1: 5}]
> ...
> 
> tarantool> box.execute('SELECT {5::name}', {{[':name'] = 1}})
> ---
> - metadata:
>   - name: COLUMN_1
>     type: map
>   rows:
>   - [{5: 1}]
> ...
> 
> To see if the parse is able to handle both map ':' and variable ':'.
Thank you! Added these tests.


Diff:

diff --git a/changelogs/unreleased/gh-4763-introduce-map-to-sql.md b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
new file mode 100644
index 000000000..013ec8f67
--- /dev/null
+++ b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
@@ -0,0 +1,4 @@
+## feature/core
+
+ * Field type MAP is now available in SQL. The syntax has also been implemented
+   to allow the creation of MAP values (gh-4763).
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 645e597fa..7411b8f67 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -3096,6 +3096,7 @@ mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
 	}
 	mpstream_flush(&stream);
 	if (is_error) {
+		region_truncate(region, used);
 		diag_set(OutOfMemory, stream.pos - stream.buf,
 			 "mpstream_flush", "stream");
 		return NULL;
@@ -3103,6 +3104,7 @@ mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
 	*size = region_used(region) - used;
 	char *map = region_join(region, *size);
 	if (map == NULL) {
+		region_truncate(region, used);
 		diag_set(OutOfMemory, *size, "region_join", "map");
 		return NULL;
 	}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 1fbf0cfac..998acadea 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1115,7 +1115,7 @@ expr(A) ::= LB(X) exprlist(Y) RB(E). {
 
 expr(A) ::= LCB(X) maplist(Y) RCB(E). {
   struct sql *db = pParse->db;
-  struct Expr *expr = sql_expr_new_dequoted(db, TK_MAP, NULL);
+  struct Expr *expr = sql_expr_new_anon(db, TK_MAP);
   if (expr == NULL) {
     sql_expr_list_delete(db, Y);
     pParse->is_aborted = true;
@@ -1129,7 +1129,9 @@ expr(A) ::= LCB(X) maplist(Y) RCB(E). {
 }
 
 maplist(A) ::= nmaplist(A).
-maplist(A) ::= .                                  {A = 0;}
+maplist(A) ::= . {
+  A = NULL;
+}
 nmaplist(A) ::= nmaplist(A) COMMA expr(X) COLON expr(Y). {
   A = sql_expr_list_append(pParse->db, A, X.pExpr);
   A = sql_expr_list_append(pParse->db, A, Y.pExpr);
@@ -1139,9 +1141,9 @@ nmaplist(A) ::= expr(X) COLON expr(Y). {
   A = sql_expr_list_append(pParse->db, A, Y.pExpr);
 }
 
-%type maplist {ExprList*}
+%type maplist {ExprList *}
 %destructor maplist {sql_expr_list_delete(pParse->db, $$);}
-%type nmaplist {ExprList*}
+%type nmaplist {ExprList *}
 %destructor nmaplist {sql_expr_list_delete(pParse->db, $$);}
 
 expr(A) ::= TRIM(X) LP trim_operands(Y) RP(E). {
diff --git a/test/sql-tap/map.test.lua b/test/sql-tap/map.test.lua
index de1e495f3..7791ca779 100755
--- a/test/sql-tap/map.test.lua
+++ b/test/sql-tap/map.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(126)
+test:plan(131)
 
 box.schema.func.create('M1', {
     language = 'Lua',
@@ -1011,6 +1011,25 @@ test:do_execsql_test(
         "map"
     })
 
+test:do_execsql_test(
+    "map-13.4",
+    [[
+        SELECT printf({});
+    ]], {
+        '{}'
+    })
+
+local map = {[0] = 0}
+local str = '0: 0'
+for i = 1, 1000 do map[i] = i str = str .. string.format(', %d: %d', i, i) end
+test:do_execsql_test(
+    "map-13.5",
+    [[
+        SELECT {]]..str..[[};
+    ]], {
+        map
+    })
+
 -- Make sure MAP() accepts only INTEGER, STRING and UUID as keys.
 test:do_execsql_test(
     "map-13.4",
@@ -1118,6 +1137,34 @@ test:do_test(
         "Type mismatch: can not convert double(1.5) to integer, string or uuid"
     })
 
+-- Make sure symbol ':' is properly processed by parser.
+test:do_test(
+    "map-14.1",
+    function()
+        local res = {pcall(box.execute, [[SELECT {:name};]], {{[':name'] = 1}})}
+        return {tostring(res[3])}
+    end, {
+        "Syntax error at line 1 near '}'"
+    })
+
+test:do_test(
+    "map-14.2",
+    function()
+        local res = box.execute([[SELECT {:name: 5}]], {{[':name'] = 1}})
+        return {tostring(res.rows[1])}
+    end, {
+        "[{1: 5}]"
+    })
+
+test:do_test(
+    "map-14.3",
+    function()
+        local res = box.execute([[SELECT {5::name}]], {{[':name'] = 1}})
+        return {tostring(res.rows[1])}
+    end, {
+        "[{5: 1}]"
+    })
+
 box.execute([[DROP TABLE t1;]])
 box.execute([[DROP TABLE t;]])
 

New patch:

commit 3cf1a8e69375fe7627439233f1f67dcc207f6d0b
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Nov 18 11:07:59 2021 +0300

    sql: introduce syntax for MAP values
    
    This patch introduces a new syntax that allows to create MAP values in
    an SQL query.
    
    Part of #4763
    
    @TarantoolBot document
    Title: Syntax for MAP in SQL
    
    The syntax for creating document values is available in SQL. You can use
    `{`, ':' and `}` to create a MAP value. Only INTEGER, STRING and UUID
    values can be keys in MAP values.
    
    Examples:
    ```
    tarantool> box.execute("SELECT {1 : 'a', 'asd' : 1.5, uuid() : true};")
    ---
    - metadata:
      - name: COLUMN_1
        type: map
      rows:
      - [{1: 'a', 91ca4dbb-c6d4-4468-b4a4-ab1e409dd87e: true, 'asd': 1.5}]
    ...
    ```
    
    ```
    tarantool> box.execute("SELECT {'h' : ['abc', 321], 7 : {'b' : 1.5}};")
    ---
    - metadata:
      - name: COLUMN_1
        type: map
      rows:
      - [{7: {'b': 1.5}, 'h': ['abc', 321]}]
    ...
    ```

diff --git a/changelogs/unreleased/gh-4763-introduce-map-to-sql.md b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
new file mode 100644
index 000000000..013ec8f67
--- /dev/null
+++ b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
@@ -0,0 +1,4 @@
+## feature/core
+
+ * Field type MAP is now available in SQL. The syntax has also been implemented
+   to allow the creation of MAP values (gh-4763).
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 8ff01dd33..e4c1dcff1 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3439,6 +3439,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
 	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
 }
 
+static void
+expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
+{
+	struct Vdbe *vdbe = parser->pVdbe;
+	struct ExprList *list = expr->x.pList;
+	if (list == NULL) {
+		sqlVdbeAddOp3(vdbe, OP_Map, 0, reg, 0);
+		return;
+	}
+	int count = list->nExpr;
+	assert(count % 2 == 0);
+	for (int i = 0; i < count / 2; ++i) {
+		struct Expr *expr = list->a[2 * i].pExpr;
+		enum field_type type = sql_expr_type(expr);
+		if (expr->op != TK_VARIABLE && type != FIELD_TYPE_INTEGER &&
+		    type != FIELD_TYPE_UNSIGNED && type != FIELD_TYPE_STRING &&
+		    type != FIELD_TYPE_UUID) {
+			diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Only "
+				 "integer, string and uuid can be keys in map");
+			parser->is_aborted = true;
+			return;
+		}
+	}
+	int values_reg = parser->nMem + 1;
+	parser->nMem += count;
+	sqlExprCodeExprList(parser, list, values_reg, 0, SQL_ECEL_FACTOR);
+	sqlVdbeAddOp3(vdbe, OP_Map, count, reg, values_reg);
+}
+
 /*
  * Erase column-cache entry number i
  */
@@ -3894,6 +3923,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 		expr_code_array(pParse, pExpr, target);
 		break;
 
+	case TK_MAP:
+		expr_code_map(pParse, pExpr, target);
+		return target;
+
 	case TK_LT:
 	case TK_LE:
 	case TK_GT:
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 32b8825bc..7411b8f67 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -3070,6 +3070,47 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
 	return array;
 }
 
+char *
+mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
+	       struct region *region)
+{
+	assert(count % 2 == 0);
+	size_t used = region_used(region);
+	bool is_error = false;
+	struct mpstream stream;
+	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
+		      set_encode_error, &is_error);
+	mpstream_encode_map(&stream, (count + 1) / 2);
+	for (uint32_t i = 0; i < count / 2; ++i) {
+		const struct Mem *key = &mems[2 * i];
+		const struct Mem *value = &mems[2 * i + 1];
+		if (mem_is_metatype(key) ||
+		    (key->type & (MEM_TYPE_UINT | MEM_TYPE_INT | MEM_TYPE_UUID |
+				  MEM_TYPE_STR)) == 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 mem_str(key), "integer, string or uuid");
+			return NULL;
+		}
+		mem_to_mpstream(key, &stream);
+		mem_to_mpstream(value, &stream);
+	}
+	mpstream_flush(&stream);
+	if (is_error) {
+		region_truncate(region, used);
+		diag_set(OutOfMemory, stream.pos - stream.buf,
+			 "mpstream_flush", "stream");
+		return NULL;
+	}
+	*size = region_used(region) - used;
+	char *map = region_join(region, *size);
+	if (map == NULL) {
+		region_truncate(region, used);
+		diag_set(OutOfMemory, *size, "region_join", "map");
+		return NULL;
+	}
+	return map;
+}
+
 /**
  * Allocate a sequence of initialized vdbe memory registers
  * on region.
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 7f5ecf954..7e35123ca 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -874,3 +874,19 @@ mem_to_mpstream(const struct Mem *var, struct mpstream *stream);
 char *
 mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
 		 struct region *region);
+
+/**
+ * Encode array of MEMs as msgpack map on region. Values in even position are
+ * treated as keys in MAP, values in odd position are treated as values in MAP.
+ * number of MEMs should be even.
+ *
+ * @param mems array of MEMs to encode.
+ * @param count number of elements in the array.
+ * @param[out] size Size of encoded msgpack map.
+ * @param region Region to use.
+ * @retval NULL on error, diag message is set.
+ * @retval Pointer to valid msgpack map on success.
+ */
+char *
+mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
+	       struct region *region);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 15f6223b0..998acadea 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1075,11 +1075,11 @@ expr(A) ::= VARNUM(X). {
   A.pExpr = expr_new_variable(pParse, &X, NULL);
   spanSet(&A, &X, &X);
 }
-expr(A) ::= VARIABLE(X) id(Y). {
+expr(A) ::= COLON|VARIABLE(X) id(Y).     {
   A.pExpr = expr_new_variable(pParse, &X, &Y);
   spanSet(&A, &X, &Y);
 }
-expr(A) ::= VARIABLE(X) INTEGER(Y). {
+expr(A) ::= COLON|VARIABLE(X) INTEGER(Y).     {
   A.pExpr = expr_new_variable(pParse, &X, &Y);
   spanSet(&A, &X, &Y);
 }
@@ -1113,6 +1113,39 @@ expr(A) ::= LB(X) exprlist(Y) RB(E). {
   spanSet(&A, &X, &E);
 }
 
+expr(A) ::= LCB(X) maplist(Y) RCB(E). {
+  struct sql *db = pParse->db;
+  struct Expr *expr = sql_expr_new_anon(db, TK_MAP);
+  if (expr == NULL) {
+    sql_expr_list_delete(db, Y);
+    pParse->is_aborted = true;
+    return;
+  }
+  expr->x.pList = Y;
+  expr->type = FIELD_TYPE_MAP;
+  sqlExprSetHeightAndFlags(pParse, expr);
+  A.pExpr = expr;
+  spanSet(&A, &X, &E);
+}
+
+maplist(A) ::= nmaplist(A).
+maplist(A) ::= . {
+  A = NULL;
+}
+nmaplist(A) ::= nmaplist(A) COMMA expr(X) COLON expr(Y). {
+  A = sql_expr_list_append(pParse->db, A, X.pExpr);
+  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
+}
+nmaplist(A) ::= expr(X) COLON expr(Y). {
+  A = sql_expr_list_append(pParse->db, NULL, X.pExpr);
+  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
+}
+
+%type maplist {ExprList *}
+%destructor maplist {sql_expr_list_delete(pParse->db, $$);}
+%type nmaplist {ExprList *}
+%destructor nmaplist {sql_expr_list_delete(pParse->db, $$);}
+
 expr(A) ::= TRIM(X) LP trim_operands(Y) RP(E). {
   A.pExpr = sqlExprFunction(pParse, Y, &X);
   spanSet(&A, &X, &E);
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 8bc519b9d..9e85801a3 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -58,7 +58,9 @@
 #define CC_KYWD       1		/* Alphabetics or '_'.  Usable in a keyword */
 #define CC_ID         2		/* unicode characters usable in IDs */
 #define CC_DIGIT      3		/* Digits */
-/** SQL variables: '@', '#', ':', and '$'. */
+/** Character ':'. */
+#define CC_COLON      4
+/** SQL variable special characters: '@', '#', and '$'. */
 #define CC_VARALPHA   5
 #define CC_VARNUM     6		/* '?'.  Numeric SQL variables */
 #define CC_SPACE      7		/* Space characters */
@@ -85,17 +87,21 @@
 #define CC_LINEFEED  28		/* '\n' */
 #define CC_LB        29		/* '[' */
 #define CC_RB        30		/* ']' */
+/** Character '{'. */
+#define CC_LCB       31
+/** Character '}'. */
+#define CC_RCB       32
 
 static const char sql_ascii_class[] = {
 /*       x0  x1  x2  x3  x4  x5  x6  x7  x8 x9  xa xb  xc xd xe  xf */
 /* 0x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 7, 28, 7, 7, 7, 27, 27,
 /* 1x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27,
 /* 2x */ 7, 15, 9, 5, 5, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
-/* 3x */ 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 5, 19, 12, 14, 13, 6,
+/* 3x */ 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 4, 19, 12, 14, 13, 6,
 /* 4x */ 5, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
 /* 5x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 29, 27, 30, 27, 1,
 /* 6x */ 27, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
-/* 7x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 27, 10, 27, 25, 27,
+/* 7x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 31, 10, 32, 25, 27,
 /* 8x */ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
 /* 9x */ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
 /* Ax */ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
@@ -228,6 +234,12 @@ sql_token(const char *z, int *type, bool *is_reserved)
 	case CC_RB:
 		*type = TK_RB;
 		return 1;
+	case CC_LCB:
+		*type = TK_LCB;
+		return 1;
+	case CC_RCB:
+		*type = TK_RCB;
+		return 1;
 	case CC_SEMI:
 		*type = TK_SEMI;
 		return 1;
@@ -371,6 +383,9 @@ sql_token(const char *z, int *type, bool *is_reserved)
 	case CC_VARNUM:
 		*type = TK_VARNUM;
 		return 1;
+	case CC_COLON:
+		*type = TK_COLON;
+		return 1;
 	case CC_VARALPHA:
 		*type = TK_VARIABLE;
 		return 1;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 55e494332..86de3f98a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1438,6 +1438,26 @@ case OP_Array: {
 	break;
 }
 
+/**
+ * Opcode: Map P1 P2 P3 * *
+ * Synopsis: r[P2] = map(P3@P1)
+ *
+ * Construct an MAP value from P1 registers starting at reg(P3).
+ */
+case OP_Map: {
+	pOut = &aMem[pOp->p2];
+
+	uint32_t size;
+	struct region *region = &fiber()->gc;
+	size_t svp = region_used(region);
+	char *val = mem_encode_map(&aMem[pOp->p3], pOp->p1, &size, region);
+	if (val == NULL || mem_copy_map(pOut, val, size) != 0) {
+		region_truncate(region, svp);
+		goto abort_due_to_error;
+	}
+	break;
+}
+
 /* Opcode: Eq P1 P2 P3 P4 P5
  * Synopsis: IF r[P3]==r[P1]
  *
diff --git a/test/sql-tap/map.test.lua b/test/sql-tap/map.test.lua
index 1afbb2b1d..7791ca779 100755
--- a/test/sql-tap/map.test.lua
+++ b/test/sql-tap/map.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(110)
+test:plan(131)
 
 box.schema.func.create('M1', {
     language = 'Lua',
@@ -982,6 +982,189 @@ test:do_catchsql_test(
         1, "Failed to execute SQL statement: wrong arguments for function ZEROBLOB()"
     })
 
+-- Make sure syntax for MAP values works as intended.
+test:do_execsql_test(
+    "map-13.1",
+    [[
+        SELECT {'a': a, 'g': g, 't': t, 'n': n, 'f': f, 'i': i, 'b': b, 'v': v,
+                's': s, 'd': d, 'u': u} FROM t1 WHERE id = 1;
+    ]], {
+        {t = "1", f = 1, n = 1, v = "1", g = 1, b = true, s = 1,
+         d = require('decimal').new(1), a = {a = 1}, i = 1,
+         u = require('uuid').fromstr('11111111-1111-1111-1111-111111111111')}
+    })
+
+test:do_execsql_test(
+    "map-13.2",
+    [[
+        SELECT {'q': 1, 'w': true, 'e': 1.5e0, 'r': ['asd', x'32'], 't': 123.0};
+    ]], {
+        {w = true, e = 1.5, r = {'asd', '2'}, t = require('decimal').new(123),
+         q = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.3",
+    [[
+        SELECT typeof({1: 1});
+    ]], {
+        "map"
+    })
+
+test:do_execsql_test(
+    "map-13.4",
+    [[
+        SELECT printf({});
+    ]], {
+        '{}'
+    })
+
+local map = {[0] = 0}
+local str = '0: 0'
+for i = 1, 1000 do map[i] = i str = str .. string.format(', %d: %d', i, i) end
+test:do_execsql_test(
+    "map-13.5",
+    [[
+        SELECT {]]..str..[[};
+    ]], {
+        map
+    })
+
+-- Make sure MAP() accepts only INTEGER, STRING and UUID as keys.
+test:do_execsql_test(
+    "map-13.4",
+    [[
+        SELECT {1: 1};
+    ]], {
+        {[1] = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.5",
+    [[
+        SELECT {-1: 1};
+    ]], {
+        {[-1] = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.6",
+    [[
+        SELECT {'a': 1};
+    ]], {
+        {a = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.6",
+    [[
+        SELECT typeof({UUID(): 1});
+    ]], {
+        "map"
+    })
+
+test:do_catchsql_test(
+    "map-13.7",
+    [[
+        SELECT {1.5e0: 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.8",
+    [[
+        SELECT {1.5: 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.9",
+    [[
+        SELECT {x'33': 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.10",
+    [[
+        SELECT {[1, 2, 3]: 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.11",
+    [[
+        SELECT {{'a': 1}: 1};
+    ]], {
+        1,
+        'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_catchsql_test(
+    "map-13.12",
+    [[
+        SELECT {CAST(1 AS NUMBER): 1};
+    ]], {
+        1, 'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_catchsql_test(
+    "map-13.13",
+    [[
+        SELECT {CAST(1 AS SCALAR): 1};
+    ]], {
+        1, 'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_catchsql_test(
+    "map-13.14",
+    [[
+        SELECT {CAST(1 AS ANY): 1};
+    ]], {
+        1, 'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_test(
+    "map-13.15",
+    function()
+        local res = {pcall(box.execute, [[SELECT {?: 1};]], {1.5})}
+        return {tostring(res[3])}
+    end, {
+        "Type mismatch: can not convert double(1.5) to integer, string or uuid"
+    })
+
+-- Make sure symbol ':' is properly processed by parser.
+test:do_test(
+    "map-14.1",
+    function()
+        local res = {pcall(box.execute, [[SELECT {:name};]], {{[':name'] = 1}})}
+        return {tostring(res[3])}
+    end, {
+        "Syntax error at line 1 near '}'"
+    })
+
+test:do_test(
+    "map-14.2",
+    function()
+        local res = box.execute([[SELECT {:name: 5}]], {{[':name'] = 1}})
+        return {tostring(res.rows[1])}
+    end, {
+        "[{1: 5}]"
+    })
+
+test:do_test(
+    "map-14.3",
+    function()
+        local res = box.execute([[SELECT {5::name}]], {{[':name'] = 1}})
+        return {tostring(res.rows[1])}
+    end, {
+        "[{5: 1}]"
+    })
+
 box.execute([[DROP TABLE t1;]])
 box.execute([[DROP TABLE t;]])
 

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

* Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
  2021-11-25  8:33     ` Mergen Imeev via Tarantool-patches
@ 2021-11-30 22:02       ` Vladislav Shpilevoy via Tarantool-patches
  2021-12-02  8:32         ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-30 22:02 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Thanks for the fixes!

>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>> index eb169aeb8..74a98c550 100644
>>> --- a/src/box/sql/expr.c
>>> +++ b/src/box/sql/expr.c
>>> @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
>>>  	}
>>>  }
>>>  
>>> +struct Expr *
>>> +expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
>>
>> 1. You might want to call it expr_new_variable(). Or sql_expr_new_variable().
>> To be consistent with our naming policy for constructors allocating memory
>> and for consistency with with sql_expr_new_column(), sql_expr_new(),
>> sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon().
>>
> Thank you! I renamed it to expr_new_variable(). I believe we should drop 'sql_'
> prefix for functions that only accessible in SQL.

It would work for static functions. But if a function is visible in other
modules as a symbol, then you would get a conflict during linking if we
ever introduce another 'struct expr' somewhere. Even if they do not interest
anywhere in the code. However I don't mind leaving it as is. It can be fixed
later if ever needed.

See 5 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index eb169aeb8..8ff01dd33 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -1314,6 +1314,59 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
>  	}
>  }
>  
> +struct Expr *
> +expr_new_variable(struct Parse *parse, const struct Token *spec,
> +		  const struct Token *id)
> +{
> +	assert(spec != 0 && spec->n == 1);
> +	uint32_t len = 1;
> +	if (parse->parse_only) {
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
> +			 parse->line_count, parse->line_pos,
> +			 "bindings are not allowed in DDL");
> +		parse->is_aborted = true;
> +		return NULL;
> +	}
> +	if (id != NULL) {
> +		assert(spec->z[0] != '?');
> +		if (id->z - spec->z != 1) {
> +			diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN,
> +				 parse->line_count, spec->z - parse->zTail + 1,
> +				 spec->n, spec->z);
> +			parse->is_aborted = true;
> +			return NULL;
> +		}
> +		if (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) {

1. You already checked for 'id != NULL' several lines above. Also you
don't need () around the second &&.

> +			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
> +				 parse->line_count, spec->n, spec->z);
> +			parse->is_aborted = true;
> +			return NULL;
> +		}
> +		len += id->n;
> +	}
> +	struct Expr *expr = sqlDbMallocRawNN(parse->db,
> +					     sizeof(*expr) + len + 1);

2. sql_expr_new_empty().

> +	if (expr == NULL) {
> +		parse->is_aborted = true;
> +		return NULL;
> +	}
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index ee319d5ad..15f6223b0 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1019,20 +1010,16 @@ idlist(A) ::= nm(Y). {
>        p->flags = EP_Leaf;
>        p->iAgg = -1;
>        p->u.zToken = (char*)&p[1];
> -      if (op != TK_VARIABLE) {
> -        int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
> -        if (rc > name_sz) {
> -          name_sz = rc;
> -          p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
> -          if (p == NULL)
> -            goto tarantool_error;
> -          p->u.zToken = (char *) &p[1];
> -          if (sql_normalize_name(p->u.zToken, name_sz, t.z, t.n) > name_sz)
> -              unreachable();
> +      int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
> +      if (rc > name_sz) {
> +        name_sz = rc;
> +        p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
> +        if (p == NULL) {
> +          sqlDbFree(pParse->db, p);

3. From the name sqlDbReallocOrFree() it looks like 'p' is already
freed. Also it is NULL here anyway, what makes sqlDbFree() nop.

> +          pParse->is_aborted = true;

4. You will get a crash below, because p is NULL but you dereference
it right afterwards.

>          }
> -      } else {
> -        memcpy(p->u.zToken, t.z, t.n);
> -        p->u.zToken[t.n] = 0;
> +        p->u.zToken = (char *) &p[1];

5. Don't need whitespace after cast operator.

> +        sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
>        }

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

* Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values
  2021-11-25  8:55     ` Mergen Imeev via Tarantool-patches
@ 2021-11-30 22:04       ` Vladislav Shpilevoy via Tarantool-patches
  2021-12-02  8:38         ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-30 22:04 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Thanks for the fixes!

On 25.11.2021 09:55, Mergen Imeev wrote:
> Thank you for the review! My answers, diff and new patch below. Also, I added
> changelog and tests to show that it is possible to create an empty MAP and a
> map with more than 1000 key-value pairs.
> 
> On Sat, Nov 20, 2021 at 01:46:57AM +0100, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> See 7 comments below.
>>
>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>> index 74a98c550..789d8906c 100644
>>> --- a/src/box/sql/expr.c
>>> +++ b/src/box/sql/expr.c
>>> @@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
>>>  	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
>>>  }
>>>  
>>> +static void
>>> +expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
>>
>> 1. I thought the policy was that we name functions, generating VDBE code,
>> using 'emit' suffix. For instance, `vdbe_emit_map()` or `sql_emit_map()`.
>> Don't know about prefix though. I see both vdbe_ and sql_ are used.
>>
> This is usually true, but this function is actually part of sqlExprCodeTarget().
> I believe these functions were created to make sqlExprCodeTarget() more
> readable. All such functions are named sqlExprCode*(), code*() or
> expr_code _*(), for example: sqlExprCodeGetColumn(), codeReal(),
> expr_code_int().
> 
> Since all these functions are static, I think we should drop "expr_" prefix for
> them. Not in this patch, though.

If functions take Expr as an argument like these do, they could be
considered methods of Expr. In that case dropping the expr_ prefix would
violate our naming convention. It is not about static or global here.

As an alternative they could be considered as methods of Parse, but
then they would need to have parse_ prefix.

For 'code' vs 'emit' - 'code' is fine by me as long as it is static. But
if it goes public, then either 'code' or 'emit' must be chosen as one
correct suffix. Not a mix.

See 2 comments below.

> diff --git a/changelogs/unreleased/gh-4763-introduce-map-to-sql.md b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
> new file mode 100644
> index 000000000..013ec8f67
> --- /dev/null
> +++ b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
> @@ -0,0 +1,4 @@
> +## feature/core

1. I noticed just now - it should be feature/sql, not core. In
other patches, which are not yet submitted, too. If there are
any similar mistakes.

> +
> + * Field type MAP is now available in SQL. The syntax has also been implemented
> +   to allow the creation of MAP values (gh-4763).> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 32b8825bc..7411b8f67 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -3070,6 +3070,47 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
>  	return array;
>  }
>  
> +char *
> +mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
> +	       struct region *region)
> +{
> +	assert(count % 2 == 0);
> +	size_t used = region_used(region);
> +	bool is_error = false;
> +	struct mpstream stream;
> +	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> +		      set_encode_error, &is_error);
> +	mpstream_encode_map(&stream, (count + 1) / 2);
> +	for (uint32_t i = 0; i < count / 2; ++i) {
> +		const struct Mem *key = &mems[2 * i];
> +		const struct Mem *value = &mems[2 * i + 1];
> +		if (mem_is_metatype(key) ||
> +		    (key->type & (MEM_TYPE_UINT | MEM_TYPE_INT | MEM_TYPE_UUID |
> +				  MEM_TYPE_STR)) == 0) {

2. Missed region truncate here. Looks like it would be easier to
add an 'error:' label in the end of the function to do the truncate
and return NULL.

> +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +				 mem_str(key), "integer, string or uuid");
> +			return NULL;
> +		}

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

* Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
  2021-11-30 22:02       ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-12-02  8:32         ` Mergen Imeev via Tarantool-patches
  2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-12-02  8:32 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review! And I'm sorry for such poor quality of my
self-review this time. I will try to avoid this in future. My asnwers,
diff and new patch below.

On Tue, Nov 30, 2021 at 11:02:40PM +0100, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> >>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> >>> index eb169aeb8..74a98c550 100644
> >>> --- a/src/box/sql/expr.c
> >>> +++ b/src/box/sql/expr.c
> >>> @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
> >>>  	}
> >>>  }
> >>>  
> >>> +struct Expr *
> >>> +expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
> >>
> >> 1. You might want to call it expr_new_variable(). Or sql_expr_new_variable().
> >> To be consistent with our naming policy for constructors allocating memory
> >> and for consistency with with sql_expr_new_column(), sql_expr_new(),
> >> sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon().
> >>
> > Thank you! I renamed it to expr_new_variable(). I believe we should drop 'sql_'
> > prefix for functions that only accessible in SQL.
> 
> It would work for static functions. But if a function is visible in other
> modules as a symbol, then you would get a conflict during linking if we
> ever introduce another 'struct expr' somewhere. Even if they do not interest
> anywhere in the code. However I don't mind leaving it as is. It can be fixed
> later if ever needed.
> 
I agree. However, I think we need to rework all the places where BOX uses
internal SQL functions and structures. In this case, the struct expr should
never be available in the BOX, so there should be no conflicts.

> See 5 comments below.
> 
> > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> > index eb169aeb8..8ff01dd33 100644
> > --- a/src/box/sql/expr.c
> > +++ b/src/box/sql/expr.c
> > @@ -1314,6 +1314,59 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
> >  	}
> >  }
> >  
> > +struct Expr *
> > +expr_new_variable(struct Parse *parse, const struct Token *spec,
> > +		  const struct Token *id)
> > +{
> > +	assert(spec != 0 && spec->n == 1);
> > +	uint32_t len = 1;
> > +	if (parse->parse_only) {
> > +		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
> > +			 parse->line_count, parse->line_pos,
> > +			 "bindings are not allowed in DDL");
> > +		parse->is_aborted = true;
> > +		return NULL;
> > +	}
> > +	if (id != NULL) {
> > +		assert(spec->z[0] != '?');
> > +		if (id->z - spec->z != 1) {
> > +			diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN,
> > +				 parse->line_count, spec->z - parse->zTail + 1,
> > +				 spec->n, spec->z);
> > +			parse->is_aborted = true;
> > +			return NULL;
> > +		}
> > +		if (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) {
> 
> 1. You already checked for 'id != NULL' several lines above. Also you
> don't need () around the second &&.
> 
Thanks, fixed.

> > +			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
> > +				 parse->line_count, spec->n, spec->z);
> > +			parse->is_aborted = true;
> > +			return NULL;
> > +		}
> > +		len += id->n;
> > +	}
> > +	struct Expr *expr = sqlDbMallocRawNN(parse->db,
> > +					     sizeof(*expr) + len + 1);
> 
> 2. sql_expr_new_empty().
> 
Fixed.

> > +	if (expr == NULL) {
> > +		parse->is_aborted = true;
> > +		return NULL;
> > +	}
> > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> > index ee319d5ad..15f6223b0 100644
> > --- a/src/box/sql/parse.y
> > +++ b/src/box/sql/parse.y
> > @@ -1019,20 +1010,16 @@ idlist(A) ::= nm(Y). {
> >        p->flags = EP_Leaf;
> >        p->iAgg = -1;
> >        p->u.zToken = (char*)&p[1];
> > -      if (op != TK_VARIABLE) {
> > -        int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
> > -        if (rc > name_sz) {
> > -          name_sz = rc;
> > -          p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
> > -          if (p == NULL)
> > -            goto tarantool_error;
> > -          p->u.zToken = (char *) &p[1];
> > -          if (sql_normalize_name(p->u.zToken, name_sz, t.z, t.n) > name_sz)
> > -              unreachable();
> > +      int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
> > +      if (rc > name_sz) {
> > +        name_sz = rc;
> > +        p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
> > +        if (p == NULL) {
> > +          sqlDbFree(pParse->db, p);
> 
> 3. From the name sqlDbReallocOrFree() it looks like 'p' is already
> freed. Also it is NULL here anyway, what makes sqlDbFree() nop.
> 
Fixed.

> > +          pParse->is_aborted = true;
> 
> 4. You will get a crash below, because p is NULL but you dereference
> it right afterwards.
> 

Fixed.

> >          }
> > -      } else {
> > -        memcpy(p->u.zToken, t.z, t.n);
> > -        p->u.zToken[t.n] = 0;
> > +        p->u.zToken = (char *) &p[1];
> 
> 5. Don't need whitespace after cast operator.
> 
Fixed.

> > +        sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
> >        }


Diff:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 8ff01dd33..e832984c3 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1318,7 +1318,7 @@ struct Expr *
 expr_new_variable(struct Parse *parse, const struct Token *spec,
 		  const struct Token *id)
 {
-	assert(spec != 0 && spec->n == 1);
+	assert(spec != NULL && spec->n == 1);
 	uint32_t len = 1;
 	if (parse->parse_only) {
 		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
@@ -1336,7 +1336,7 @@ expr_new_variable(struct Parse *parse, const struct Token *spec,
 			parse->is_aborted = true;
 			return NULL;
 		}
-		if (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) {
+		if (spec->z[0] == '#' && sqlIsdigit(id->z[0])) {
 			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
 				 parse->line_count, spec->n, spec->z);
 			parse->is_aborted = true;
@@ -1344,15 +1344,8 @@ expr_new_variable(struct Parse *parse, const struct Token *spec,
 		}
 		len += id->n;
 	}
-	struct Expr *expr = sqlDbMallocRawNN(parse->db,
-					     sizeof(*expr) + len + 1);
-	if (expr == NULL) {
-		parse->is_aborted = true;
-		return NULL;
-	}
-	memset(expr, 0, sizeof(*expr));
+	struct Expr *expr = sql_expr_new_empty(parse->db, TK_VARIABLE, len + 1);
 	expr->type = FIELD_TYPE_BOOLEAN;
-	expr->op = TK_VARIABLE;
 	expr->flags = EP_Leaf;
 	expr->iAgg = -1;
 	expr->u.zToken = (char *)(expr + 1);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 15f6223b0..ecc7f7778 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1015,10 +1015,10 @@ idlist(A) ::= nm(Y). {
         name_sz = rc;
         p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
         if (p == NULL) {
-          sqlDbFree(pParse->db, p);
           pParse->is_aborted = true;
+          return;
         }
-        p->u.zToken = (char *) &p[1];
+        p->u.zToken = (char *)&p[1];
         sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
       }
 #if SQL_MAX_EXPR_DEPTH>0


New patch:

commit 9a4bfea66a2e76340e96dff4ba0b04fe54f9d681
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Nov 18 11:02:20 2021 +0300

    sql: properly check bind variable names
    
    After this patch, variable names will have to follow the rules defined
    for identifiers in SQL. This essentially means that the number will no
    longer be used as the first character of the bind variable name.
    
    Part of #4763

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index eb169aeb8..e832984c3 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 	}
 }
 
+struct Expr *
+expr_new_variable(struct Parse *parse, const struct Token *spec,
+		  const struct Token *id)
+{
+	assert(spec != NULL && spec->n == 1);
+	uint32_t len = 1;
+	if (parse->parse_only) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
+			 parse->line_count, parse->line_pos,
+			 "bindings are not allowed in DDL");
+		parse->is_aborted = true;
+		return NULL;
+	}
+	if (id != NULL) {
+		assert(spec->z[0] != '?');
+		if (id->z - spec->z != 1) {
+			diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN,
+				 parse->line_count, spec->z - parse->zTail + 1,
+				 spec->n, spec->z);
+			parse->is_aborted = true;
+			return NULL;
+		}
+		if (spec->z[0] == '#' && sqlIsdigit(id->z[0])) {
+			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
+				 parse->line_count, spec->n, spec->z);
+			parse->is_aborted = true;
+			return NULL;
+		}
+		len += id->n;
+	}
+	struct Expr *expr = sql_expr_new_empty(parse->db, TK_VARIABLE, len + 1);
+	expr->type = FIELD_TYPE_BOOLEAN;
+	expr->flags = EP_Leaf;
+	expr->iAgg = -1;
+	expr->u.zToken = (char *)(expr + 1);
+	expr->u.zToken[0] = spec->z[0];
+	if (id != NULL)
+		memcpy(expr->u.zToken + 1, id->z, id->n);
+	expr->u.zToken[len] = '\0';
+	assert(SQL_MAX_EXPR_DEPTH > 0);
+	expr->nHeight = 1;
+
+	sqlExprAssignVarNumber(parse, expr, len);
+	return expr;
+}
+
 /*
  * Recursively delete an expression tree.
  */
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ee319d5ad..ecc7f7778 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1002,15 +1002,6 @@ idlist(A) ::= nm(Y). {
       case TK_UNKNOWN:
         p->type = FIELD_TYPE_BOOLEAN;
         break;
-      case TK_VARIABLE:
-        /*
-         * For variables we set BOOLEAN type since
-         * unassigned bindings will be replaced
-         * with NULL automatically, i.e. without
-         * explicit call of sql_bind_*().
-         */
-        p->type = FIELD_TYPE_BOOLEAN;
-        break;
       default:
         p->type = FIELD_TYPE_SCALAR;
         break;
@@ -1019,20 +1010,16 @@ idlist(A) ::= nm(Y). {
       p->flags = EP_Leaf;
       p->iAgg = -1;
       p->u.zToken = (char*)&p[1];
-      if (op != TK_VARIABLE) {
-        int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
-        if (rc > name_sz) {
-          name_sz = rc;
-          p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
-          if (p == NULL)
-            goto tarantool_error;
-          p->u.zToken = (char *) &p[1];
-          if (sql_normalize_name(p->u.zToken, name_sz, t.z, t.n) > name_sz)
-              unreachable();
+      int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
+      if (rc > name_sz) {
+        name_sz = rc;
+        p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
+        if (p == NULL) {
+          pParse->is_aborted = true;
+          return;
         }
-      } else {
-        memcpy(p->u.zToken, t.z, t.n);
-        p->u.zToken[t.n] = 0;
+        p->u.zToken = (char *)&p[1];
+        sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
       }
 #if SQL_MAX_EXPR_DEPTH>0
       p->nHeight = 1;
@@ -1042,9 +1029,6 @@ idlist(A) ::= nm(Y). {
     pOut->zStart = t.z;
     pOut->zEnd = &t.z[t.n];
     return;
-tarantool_error:
-    sqlDbFree(pParse->db, p);
-    pParse->is_aborted = true;
   }
 }
 
@@ -1087,30 +1071,17 @@ term(A) ::= INTEGER(X). {
   A.zEnd = X.z + X.n;
   if( A.pExpr ) A.pExpr->flags |= EP_Leaf;
 }
-expr(A) ::= VARIABLE(X).     {
-  Token t = X;
-  if (pParse->parse_only) {
-    spanSet(&A, &t, &t);
-    diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS, pParse->line_count,
-             pParse->line_pos, "bindings are not allowed in DDL");
-    pParse->is_aborted = true;
-    A.pExpr = NULL;
-  } else if (!(X.z[0]=='#' && sqlIsdigit(X.z[1]))) {
-    u32 n = X.n;
-    spanExpr(&A, pParse, TK_VARIABLE, X);
-    if (A.pExpr->u.zToken[0] == '?' && n > 1) {
-      diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
-      pParse->is_aborted = true;
-    } else {
-      sqlExprAssignVarNumber(pParse, A.pExpr, n);
-    }
-  }else{
-    assert( t.n>=2 );
-    spanSet(&A, &t, &t);
-    diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
-    pParse->is_aborted = true;
-    A.pExpr = NULL;
-  }
+expr(A) ::= VARNUM(X). {
+  A.pExpr = expr_new_variable(pParse, &X, NULL);
+  spanSet(&A, &X, &X);
+}
+expr(A) ::= VARIABLE(X) id(Y). {
+  A.pExpr = expr_new_variable(pParse, &X, &Y);
+  spanSet(&A, &X, &Y);
+}
+expr(A) ::= VARIABLE(X) INTEGER(Y). {
+  A.pExpr = expr_new_variable(pParse, &X, &Y);
+  spanSet(&A, &X, &Y);
 }
 expr(A) ::= expr(A) COLLATE id(C). {
   A.pExpr = sqlExprAddCollateToken(pParse, A.pExpr, &C, 1);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index dcd71e5bd..1c469e3fc 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2617,6 +2617,17 @@ Expr *sqlExprFunction(Parse *, ExprList *, Token *);
 void sqlExprAssignVarNumber(Parse *, Expr *, u32);
 ExprList *sqlExprListAppendVector(Parse *, ExprList *, IdList *, Expr *);
 
+/**
+ * Parse tokens as a name or a position of bound variable.
+ *
+ * @param parse Parse context.
+ * @param spec Special symbol for bound variable.
+ * @param id Name or position number of bound variable.
+ */
+struct Expr *
+expr_new_variable(struct Parse *parse, const struct Token *spec,
+		  const struct Token *id);
+
 /**
  * Set the sort order for the last element on the given ExprList.
  *
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index f2d5a2df5..8bc519b9d 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -58,8 +58,8 @@
 #define CC_KYWD       1		/* Alphabetics or '_'.  Usable in a keyword */
 #define CC_ID         2		/* unicode characters usable in IDs */
 #define CC_DIGIT      3		/* Digits */
-#define CC_DOLLAR     4		/* '$' */
-#define CC_VARALPHA   5		/* '@', '#', ':'.  Alphabetic SQL variables */
+/** SQL variables: '@', '#', ':', and '$'. */
+#define CC_VARALPHA   5
 #define CC_VARNUM     6		/* '?'.  Numeric SQL variables */
 #define CC_SPACE      7		/* Space characters */
 #define CC_QUOTE      8		/* '\''. String literals */
@@ -90,7 +90,7 @@ static const char sql_ascii_class[] = {
 /*       x0  x1  x2  x3  x4  x5  x6  x7  x8 x9  xa xb  xc xd xe  xf */
 /* 0x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 7, 28, 7, 7, 7, 27, 27,
 /* 1x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27,
-/* 2x */ 7, 15, 9, 5, 4, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
+/* 2x */ 7, 15, 9, 5, 5, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
 /* 3x */ 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 5, 19, 12, 14, 13, 6,
 /* 4x */ 5, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
 /* 5x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 29, 27, 30, 27, 1,
@@ -184,7 +184,7 @@ int
 sql_token(const char *z, int *type, bool *is_reserved)
 {
 	*is_reserved = false;
-	int i, n;
+	int i;
 	char c, delim;
 	/* Switch on the character-class of the first byte
 	 * of the token. See the comment on the CC_ defines
@@ -369,23 +369,11 @@ sql_token(const char *z, int *type, bool *is_reserved)
 		}
 		return i;
 	case CC_VARNUM:
-		*type = TK_VARIABLE;
-		for (i = 1; sqlIsdigit(z[i]); i++) {
-		}
-		return i;
-	case CC_DOLLAR:
+		*type = TK_VARNUM;
+		return 1;
 	case CC_VARALPHA:
-		n = 0;
 		*type = TK_VARIABLE;
-		for (i = 1; (c = z[i]) != 0; i++) {
-			if (IdChar(c))
-				n++;
-			else
-				break;
-		}
-		if (n == 0)
-			*type = TK_ILLEGAL;
-		return i;
+		return 1;
 	case CC_KYWD:
 		for (i = 1; sql_ascii_class[*(unsigned char*)(z+i)] <= CC_KYWD;
 		     i++) {
diff --git a/test/sql-tap/bind.test.lua b/test/sql-tap/bind.test.lua
new file mode 100755
index 000000000..aaf14a44d
--- /dev/null
+++ b/test/sql-tap/bind.test.lua
@@ -0,0 +1,15 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(1)
+
+-- Make sure that bind variable names cannot start with a number.
+test:do_test(
+    "bind-1",
+    function()
+        local res = {pcall(box.execute, [[SELECT @1asd;]], {{['@1asd'] = 123}})}
+        return {tostring(res[3])}
+    end, {
+        "At line 1 at or near position 9: unrecognized token '1asd'"
+    })
+
+test:finish_test()
diff --git a/test/sql-tap/misc1.test.lua b/test/sql-tap/misc1.test.lua
index f207d3e92..204e070d2 100755
--- a/test/sql-tap/misc1.test.lua
+++ b/test/sql-tap/misc1.test.lua
@@ -1052,7 +1052,7 @@ test:do_catchsql_test(
         select''like''like''like#0;
     ]], {
         -- <misc1-21.1>
-        1, [[Syntax error at line 1 near '#0']]
+        1, [[Syntax error at line 1 near '#']]
         -- </misc1-21.1>
     })
 
@@ -1062,7 +1062,7 @@ test:do_catchsql_test(
         VALUES(0,0x0MATCH#0;
     ]], {
         -- <misc1-21.2>
-        1, [[Syntax error at line 1 near '#0']]
+        1, [[Syntax error at line 1 near '#']]
         -- </misc1-21.2>
     })
 
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 6212aa0c0..5a424596e 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -351,7 +351,7 @@ cn:execute('drop table if exists test3')
 --
 cn:execute('select ?1, ?2, ?3', {1, 2, 3})
 ---
-- error: Syntax error at line 1 near '?1'
+- error: Syntax error at line 1 near '1'
 ...
 cn:execute('select $name, $name2', {1, 2})
 ---

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

* Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values
  2021-11-30 22:04       ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-12-02  8:38         ` Mergen Imeev via Tarantool-patches
  2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-12-02  8:38 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

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

On Tue, Nov 30, 2021 at 11:04:47PM +0100, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> On 25.11.2021 09:55, Mergen Imeev wrote:
> > Thank you for the review! My answers, diff and new patch below. Also, I added
> > changelog and tests to show that it is possible to create an empty MAP and a
> > map with more than 1000 key-value pairs.
> > 
> > On Sat, Nov 20, 2021 at 01:46:57AM +0100, Vladislav Shpilevoy wrote:
> >> Thanks for the patch!
> >>
> >> See 7 comments below.
> >>
> >>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> >>> index 74a98c550..789d8906c 100644
> >>> --- a/src/box/sql/expr.c
> >>> +++ b/src/box/sql/expr.c
> >>> @@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
> >>>  	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
> >>>  }
> >>>  
> >>> +static void
> >>> +expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
> >>
> >> 1. I thought the policy was that we name functions, generating VDBE code,
> >> using 'emit' suffix. For instance, `vdbe_emit_map()` or `sql_emit_map()`.
> >> Don't know about prefix though. I see both vdbe_ and sql_ are used.
> >>
> > This is usually true, but this function is actually part of sqlExprCodeTarget().
> > I believe these functions were created to make sqlExprCodeTarget() more
> > readable. All such functions are named sqlExprCode*(), code*() or
> > expr_code _*(), for example: sqlExprCodeGetColumn(), codeReal(),
> > expr_code_int().
> > 
> > Since all these functions are static, I think we should drop "expr_" prefix for
> > them. Not in this patch, though.
> 
> If functions take Expr as an argument like these do, they could be
> considered methods of Expr. In that case dropping the expr_ prefix would
> violate our naming convention. It is not about static or global here.
> 
> As an alternative they could be considered as methods of Parse, but
> then they would need to have parse_ prefix.
> 
> For 'code' vs 'emit' - 'code' is fine by me as long as it is static. But
> if it goes public, then either 'code' or 'emit' must be chosen as one
> correct suffix. Not a mix.
> 
After some thought, I think you are right. However, I would suggest removing the
parser and vdbe from these functions and converting them to proper struct expr
methods. This way we can make these functions return a value (most likely as an
"out" argument). For example expr_code_dec() should give us DECIMAL. In this
case we can make some improvements, for example we can remove "is_neg" from
expr_code_int() and turn it into expr_code_uint(), since we know that this '-'
sign will be specified as another expr. Also, since these will be valid expr
methods, we can drop "static" from their definition. We then should name them
accordingly, for  example "expr_code_dec" may be named "expr_to_dec".

> See 2 comments below.
> 
> > diff --git a/changelogs/unreleased/gh-4763-introduce-map-to-sql.md b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
> > new file mode 100644
> > index 000000000..013ec8f67
> > --- /dev/null
> > +++ b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
> > @@ -0,0 +1,4 @@
> > +## feature/core
> 
> 1. I noticed just now - it should be feature/sql, not core. In
> other patches, which are not yet submitted, too. If there are
> any similar mistakes.
> 
Thank you. Fixed here and in patch-set about ARRAY syntax.

> > +
> > + * Field type MAP is now available in SQL. The syntax has also been implemented
> > +   to allow the creation of MAP values (gh-4763).> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 32b8825bc..7411b8f67 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -3070,6 +3070,47 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
> >  	return array;
> >  }
> >  
> > +char *
> > +mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
> > +	       struct region *region)
> > +{
> > +	assert(count % 2 == 0);
> > +	size_t used = region_used(region);
> > +	bool is_error = false;
> > +	struct mpstream stream;
> > +	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> > +		      set_encode_error, &is_error);
> > +	mpstream_encode_map(&stream, (count + 1) / 2);
> > +	for (uint32_t i = 0; i < count / 2; ++i) {
> > +		const struct Mem *key = &mems[2 * i];
> > +		const struct Mem *value = &mems[2 * i + 1];
> > +		if (mem_is_metatype(key) ||
> > +		    (key->type & (MEM_TYPE_UINT | MEM_TYPE_INT | MEM_TYPE_UUID |
> > +				  MEM_TYPE_STR)) == 0) {
> 
> 2. Missed region truncate here. Looks like it would be easier to
> add an 'error:' label in the end of the function to do the truncate
> and return NULL.
> 
Done.

> > +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +				 mem_str(key), "integer, string or uuid");
> > +			return NULL;
> > +		}

Diff:

diff --git a/changelogs/unreleased/gh-4763-introduce-map-to-sql.md b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
index 013ec8f67..bc078b4c2 100644
--- a/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
+++ b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
@@ -1,4 +1,4 @@
-## feature/core
+## feature/sql
 
  * Field type MAP is now available in SQL. The syntax has also been implemented
    to allow the creation of MAP values (gh-4763).
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index a275bf385..6ee5d12cc 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -3089,26 +3089,25 @@ mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
 				  MEM_TYPE_STR)) == 0) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 				 mem_str(key), "integer, string or uuid");
-			return NULL;
+			goto error;
 		}
 		mem_to_mpstream(key, &stream);
 		mem_to_mpstream(value, &stream);
 	}
 	mpstream_flush(&stream);
 	if (is_error) {
-		region_truncate(region, used);
 		diag_set(OutOfMemory, stream.pos - stream.buf,
 			 "mpstream_flush", "stream");
-		return NULL;
+		goto error;
 	}
 	*size = region_used(region) - used;
 	char *map = region_join(region, *size);
-	if (map == NULL) {
-		region_truncate(region, used);
-		diag_set(OutOfMemory, *size, "region_join", "map");
-		return NULL;
-	}
-	return map;
+	if (map != NULL)
+		return map;
+	diag_set(OutOfMemory, *size, "region_join", "map");
+error:
+	region_truncate(region, used);
+	return NULL;
 }
 
 /**


New patch:

commit 91453c29ec21ad092c869012fe1a526935ce3be5
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Nov 18 11:07:59 2021 +0300

    sql: introduce syntax for MAP values
    
    This patch introduces a new syntax that allows to create MAP values in
    an SQL query.
    
    Part of #4763
    
    @TarantoolBot document
    Title: Syntax for MAP in SQL
    
    The syntax for creating document values is available in SQL. You can use
    `{`, ':' and `}` to create a MAP value. Only INTEGER, STRING and UUID
    values can be keys in MAP values.
    
    Examples:
    ```
    tarantool> box.execute("SELECT {1 : 'a', 'asd' : 1.5, uuid() : true};")
    ---
    - metadata:
      - name: COLUMN_1
        type: map
      rows:
      - [{1: 'a', 91ca4dbb-c6d4-4468-b4a4-ab1e409dd87e: true, 'asd': 1.5}]
    ...
    ```
    
    ```
    tarantool> box.execute("SELECT {'h' : ['abc', 321], 7 : {'b' : 1.5}};")
    ---
    - metadata:
      - name: COLUMN_1
        type: map
      rows:
      - [{7: {'b': 1.5}, 'h': ['abc', 321]}]
    ...
    ```

diff --git a/changelogs/unreleased/gh-4763-introduce-map-to-sql.md b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
new file mode 100644
index 000000000..bc078b4c2
--- /dev/null
+++ b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
@@ -0,0 +1,4 @@
+## feature/sql
+
+ * Field type MAP is now available in SQL. The syntax has also been implemented
+   to allow the creation of MAP values (gh-4763).
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e832984c3..2dac9d2ef 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
 	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
 }
 
+static void
+expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
+{
+	struct Vdbe *vdbe = parser->pVdbe;
+	struct ExprList *list = expr->x.pList;
+	if (list == NULL) {
+		sqlVdbeAddOp3(vdbe, OP_Map, 0, reg, 0);
+		return;
+	}
+	int count = list->nExpr;
+	assert(count % 2 == 0);
+	for (int i = 0; i < count / 2; ++i) {
+		struct Expr *expr = list->a[2 * i].pExpr;
+		enum field_type type = sql_expr_type(expr);
+		if (expr->op != TK_VARIABLE && type != FIELD_TYPE_INTEGER &&
+		    type != FIELD_TYPE_UNSIGNED && type != FIELD_TYPE_STRING &&
+		    type != FIELD_TYPE_UUID) {
+			diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Only "
+				 "integer, string and uuid can be keys in map");
+			parser->is_aborted = true;
+			return;
+		}
+	}
+	int values_reg = parser->nMem + 1;
+	parser->nMem += count;
+	sqlExprCodeExprList(parser, list, values_reg, 0, SQL_ECEL_FACTOR);
+	sqlVdbeAddOp3(vdbe, OP_Map, count, reg, values_reg);
+}
+
 /*
  * Erase column-cache entry number i
  */
@@ -3887,6 +3916,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 		expr_code_array(pParse, pExpr, target);
 		break;
 
+	case TK_MAP:
+		expr_code_map(pParse, pExpr, target);
+		return target;
+
 	case TK_LT:
 	case TK_LE:
 	case TK_GT:
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 510a7cce2..6ee5d12cc 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -3070,6 +3070,46 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
 	return array;
 }
 
+char *
+mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
+	       struct region *region)
+{
+	assert(count % 2 == 0);
+	size_t used = region_used(region);
+	bool is_error = false;
+	struct mpstream stream;
+	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
+		      set_encode_error, &is_error);
+	mpstream_encode_map(&stream, (count + 1) / 2);
+	for (uint32_t i = 0; i < count / 2; ++i) {
+		const struct Mem *key = &mems[2 * i];
+		const struct Mem *value = &mems[2 * i + 1];
+		if (mem_is_metatype(key) ||
+		    (key->type & (MEM_TYPE_UINT | MEM_TYPE_INT | MEM_TYPE_UUID |
+				  MEM_TYPE_STR)) == 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 mem_str(key), "integer, string or uuid");
+			goto error;
+		}
+		mem_to_mpstream(key, &stream);
+		mem_to_mpstream(value, &stream);
+	}
+	mpstream_flush(&stream);
+	if (is_error) {
+		diag_set(OutOfMemory, stream.pos - stream.buf,
+			 "mpstream_flush", "stream");
+		goto error;
+	}
+	*size = region_used(region) - used;
+	char *map = region_join(region, *size);
+	if (map != NULL)
+		return map;
+	diag_set(OutOfMemory, *size, "region_join", "map");
+error:
+	region_truncate(region, used);
+	return NULL;
+}
+
 /**
  * Allocate a sequence of initialized vdbe memory registers
  * on region.
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 7f5ecf954..7e35123ca 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -874,3 +874,19 @@ mem_to_mpstream(const struct Mem *var, struct mpstream *stream);
 char *
 mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
 		 struct region *region);
+
+/**
+ * Encode array of MEMs as msgpack map on region. Values in even position are
+ * treated as keys in MAP, values in odd position are treated as values in MAP.
+ * number of MEMs should be even.
+ *
+ * @param mems array of MEMs to encode.
+ * @param count number of elements in the array.
+ * @param[out] size Size of encoded msgpack map.
+ * @param region Region to use.
+ * @retval NULL on error, diag message is set.
+ * @retval Pointer to valid msgpack map on success.
+ */
+char *
+mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
+	       struct region *region);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ecc7f7778..a426a3ccf 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1075,11 +1075,11 @@ expr(A) ::= VARNUM(X). {
   A.pExpr = expr_new_variable(pParse, &X, NULL);
   spanSet(&A, &X, &X);
 }
-expr(A) ::= VARIABLE(X) id(Y). {
+expr(A) ::= COLON|VARIABLE(X) id(Y).     {
   A.pExpr = expr_new_variable(pParse, &X, &Y);
   spanSet(&A, &X, &Y);
 }
-expr(A) ::= VARIABLE(X) INTEGER(Y). {
+expr(A) ::= COLON|VARIABLE(X) INTEGER(Y).     {
   A.pExpr = expr_new_variable(pParse, &X, &Y);
   spanSet(&A, &X, &Y);
 }
@@ -1113,6 +1113,39 @@ expr(A) ::= LB(X) exprlist(Y) RB(E). {
   spanSet(&A, &X, &E);
 }
 
+expr(A) ::= LCB(X) maplist(Y) RCB(E). {
+  struct sql *db = pParse->db;
+  struct Expr *expr = sql_expr_new_anon(db, TK_MAP);
+  if (expr == NULL) {
+    sql_expr_list_delete(db, Y);
+    pParse->is_aborted = true;
+    return;
+  }
+  expr->x.pList = Y;
+  expr->type = FIELD_TYPE_MAP;
+  sqlExprSetHeightAndFlags(pParse, expr);
+  A.pExpr = expr;
+  spanSet(&A, &X, &E);
+}
+
+maplist(A) ::= nmaplist(A).
+maplist(A) ::= . {
+  A = NULL;
+}
+nmaplist(A) ::= nmaplist(A) COMMA expr(X) COLON expr(Y). {
+  A = sql_expr_list_append(pParse->db, A, X.pExpr);
+  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
+}
+nmaplist(A) ::= expr(X) COLON expr(Y). {
+  A = sql_expr_list_append(pParse->db, NULL, X.pExpr);
+  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
+}
+
+%type maplist {ExprList *}
+%destructor maplist {sql_expr_list_delete(pParse->db, $$);}
+%type nmaplist {ExprList *}
+%destructor nmaplist {sql_expr_list_delete(pParse->db, $$);}
+
 expr(A) ::= TRIM(X) LP trim_operands(Y) RP(E). {
   A.pExpr = sqlExprFunction(pParse, Y, &X);
   spanSet(&A, &X, &E);
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 8bc519b9d..9e85801a3 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -58,7 +58,9 @@
 #define CC_KYWD       1		/* Alphabetics or '_'.  Usable in a keyword */
 #define CC_ID         2		/* unicode characters usable in IDs */
 #define CC_DIGIT      3		/* Digits */
-/** SQL variables: '@', '#', ':', and '$'. */
+/** Character ':'. */
+#define CC_COLON      4
+/** SQL variable special characters: '@', '#', and '$'. */
 #define CC_VARALPHA   5
 #define CC_VARNUM     6		/* '?'.  Numeric SQL variables */
 #define CC_SPACE      7		/* Space characters */
@@ -85,17 +87,21 @@
 #define CC_LINEFEED  28		/* '\n' */
 #define CC_LB        29		/* '[' */
 #define CC_RB        30		/* ']' */
+/** Character '{'. */
+#define CC_LCB       31
+/** Character '}'. */
+#define CC_RCB       32
 
 static const char sql_ascii_class[] = {
 /*       x0  x1  x2  x3  x4  x5  x6  x7  x8 x9  xa xb  xc xd xe  xf */
 /* 0x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 7, 28, 7, 7, 7, 27, 27,
 /* 1x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27,
 /* 2x */ 7, 15, 9, 5, 5, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
-/* 3x */ 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 5, 19, 12, 14, 13, 6,
+/* 3x */ 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 4, 19, 12, 14, 13, 6,
 /* 4x */ 5, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
 /* 5x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 29, 27, 30, 27, 1,
 /* 6x */ 27, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
-/* 7x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 27, 10, 27, 25, 27,
+/* 7x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 31, 10, 32, 25, 27,
 /* 8x */ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
 /* 9x */ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
 /* Ax */ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
@@ -228,6 +234,12 @@ sql_token(const char *z, int *type, bool *is_reserved)
 	case CC_RB:
 		*type = TK_RB;
 		return 1;
+	case CC_LCB:
+		*type = TK_LCB;
+		return 1;
+	case CC_RCB:
+		*type = TK_RCB;
+		return 1;
 	case CC_SEMI:
 		*type = TK_SEMI;
 		return 1;
@@ -371,6 +383,9 @@ sql_token(const char *z, int *type, bool *is_reserved)
 	case CC_VARNUM:
 		*type = TK_VARNUM;
 		return 1;
+	case CC_COLON:
+		*type = TK_COLON;
+		return 1;
 	case CC_VARALPHA:
 		*type = TK_VARIABLE;
 		return 1;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 55e494332..86de3f98a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1438,6 +1438,26 @@ case OP_Array: {
 	break;
 }
 
+/**
+ * Opcode: Map P1 P2 P3 * *
+ * Synopsis: r[P2] = map(P3@P1)
+ *
+ * Construct an MAP value from P1 registers starting at reg(P3).
+ */
+case OP_Map: {
+	pOut = &aMem[pOp->p2];
+
+	uint32_t size;
+	struct region *region = &fiber()->gc;
+	size_t svp = region_used(region);
+	char *val = mem_encode_map(&aMem[pOp->p3], pOp->p1, &size, region);
+	if (val == NULL || mem_copy_map(pOut, val, size) != 0) {
+		region_truncate(region, svp);
+		goto abort_due_to_error;
+	}
+	break;
+}
+
 /* Opcode: Eq P1 P2 P3 P4 P5
  * Synopsis: IF r[P3]==r[P1]
  *
diff --git a/test/sql-tap/map.test.lua b/test/sql-tap/map.test.lua
index 1afbb2b1d..7791ca779 100755
--- a/test/sql-tap/map.test.lua
+++ b/test/sql-tap/map.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(110)
+test:plan(131)
 
 box.schema.func.create('M1', {
     language = 'Lua',
@@ -982,6 +982,189 @@ test:do_catchsql_test(
         1, "Failed to execute SQL statement: wrong arguments for function ZEROBLOB()"
     })
 
+-- Make sure syntax for MAP values works as intended.
+test:do_execsql_test(
+    "map-13.1",
+    [[
+        SELECT {'a': a, 'g': g, 't': t, 'n': n, 'f': f, 'i': i, 'b': b, 'v': v,
+                's': s, 'd': d, 'u': u} FROM t1 WHERE id = 1;
+    ]], {
+        {t = "1", f = 1, n = 1, v = "1", g = 1, b = true, s = 1,
+         d = require('decimal').new(1), a = {a = 1}, i = 1,
+         u = require('uuid').fromstr('11111111-1111-1111-1111-111111111111')}
+    })
+
+test:do_execsql_test(
+    "map-13.2",
+    [[
+        SELECT {'q': 1, 'w': true, 'e': 1.5e0, 'r': ['asd', x'32'], 't': 123.0};
+    ]], {
+        {w = true, e = 1.5, r = {'asd', '2'}, t = require('decimal').new(123),
+         q = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.3",
+    [[
+        SELECT typeof({1: 1});
+    ]], {
+        "map"
+    })
+
+test:do_execsql_test(
+    "map-13.4",
+    [[
+        SELECT printf({});
+    ]], {
+        '{}'
+    })
+
+local map = {[0] = 0}
+local str = '0: 0'
+for i = 1, 1000 do map[i] = i str = str .. string.format(', %d: %d', i, i) end
+test:do_execsql_test(
+    "map-13.5",
+    [[
+        SELECT {]]..str..[[};
+    ]], {
+        map
+    })
+
+-- Make sure MAP() accepts only INTEGER, STRING and UUID as keys.
+test:do_execsql_test(
+    "map-13.4",
+    [[
+        SELECT {1: 1};
+    ]], {
+        {[1] = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.5",
+    [[
+        SELECT {-1: 1};
+    ]], {
+        {[-1] = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.6",
+    [[
+        SELECT {'a': 1};
+    ]], {
+        {a = 1}
+    })
+
+test:do_execsql_test(
+    "map-13.6",
+    [[
+        SELECT typeof({UUID(): 1});
+    ]], {
+        "map"
+    })
+
+test:do_catchsql_test(
+    "map-13.7",
+    [[
+        SELECT {1.5e0: 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.8",
+    [[
+        SELECT {1.5: 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.9",
+    [[
+        SELECT {x'33': 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.10",
+    [[
+        SELECT {[1, 2, 3]: 1};
+    ]], {
+        1, "Only integer, string and uuid can be keys in map"
+    })
+
+test:do_catchsql_test(
+    "map-13.11",
+    [[
+        SELECT {{'a': 1}: 1};
+    ]], {
+        1,
+        'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_catchsql_test(
+    "map-13.12",
+    [[
+        SELECT {CAST(1 AS NUMBER): 1};
+    ]], {
+        1, 'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_catchsql_test(
+    "map-13.13",
+    [[
+        SELECT {CAST(1 AS SCALAR): 1};
+    ]], {
+        1, 'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_catchsql_test(
+    "map-13.14",
+    [[
+        SELECT {CAST(1 AS ANY): 1};
+    ]], {
+        1, 'Only integer, string and uuid can be keys in map'
+    })
+
+test:do_test(
+    "map-13.15",
+    function()
+        local res = {pcall(box.execute, [[SELECT {?: 1};]], {1.5})}
+        return {tostring(res[3])}
+    end, {
+        "Type mismatch: can not convert double(1.5) to integer, string or uuid"
+    })
+
+-- Make sure symbol ':' is properly processed by parser.
+test:do_test(
+    "map-14.1",
+    function()
+        local res = {pcall(box.execute, [[SELECT {:name};]], {{[':name'] = 1}})}
+        return {tostring(res[3])}
+    end, {
+        "Syntax error at line 1 near '}'"
+    })
+
+test:do_test(
+    "map-14.2",
+    function()
+        local res = box.execute([[SELECT {:name: 5}]], {{[':name'] = 1}})
+        return {tostring(res.rows[1])}
+    end, {
+        "[{1: 5}]"
+    })
+
+test:do_test(
+    "map-14.3",
+    function()
+        local res = box.execute([[SELECT {5::name}]], {{[':name'] = 1}})
+        return {tostring(res.rows[1])}
+    end, {
+        "[{5: 1}]"
+    })
+
 box.execute([[DROP TABLE t1;]])
 box.execute([[DROP TABLE t;]])
 

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

* Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
  2021-12-02  8:32         ` Mergen Imeev via Tarantool-patches
@ 2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
  2021-12-13  7:34             ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-12-09  0:31 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi! Thanks for the fixes!

>>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>>>> index eb169aeb8..74a98c550 100644
>>>>> --- a/src/box/sql/expr.c
>>>>> +++ b/src/box/sql/expr.c
>>>>> @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
>>>>>  	}
>>>>>  }
>>>>>  
>>>>> +struct Expr *
>>>>> +expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
>>>>
>>>> 1. You might want to call it expr_new_variable(). Or sql_expr_new_variable().
>>>> To be consistent with our naming policy for constructors allocating memory
>>>> and for consistency with with sql_expr_new_column(), sql_expr_new(),
>>>> sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon().
>>>>
>>> Thank you! I renamed it to expr_new_variable(). I believe we should drop 'sql_'
>>> prefix for functions that only accessible in SQL.
>>
>> It would work for static functions. But if a function is visible in other
>> modules as a symbol, then you would get a conflict during linking if we
>> ever introduce another 'struct expr' somewhere. Even if they do not interest
>> anywhere in the code. However I don't mind leaving it as is. It can be fixed
>> later if ever needed.
>>
> I agree. However, I think we need to rework all the places where BOX uses
> internal SQL functions and structures. In this case, the struct expr should
> never be available in the BOX, so there should be no conflicts.

It is a misunderstanding. It does not matter if you use a function in box or
not. If it is not static and is defined in 2 places - you will get a conflict
during link stage. Try to add a function with the same name to any file in
sql and to any file in box. Something like

	void
	link_test123(void)
	{
		printf("in link test\n");
	}

(in a .cc file you would need to add 'extern "C"' for it). It will raise an
error during build. I added it to expr.c and to box.cc (with 'extern "C"'):

	duplicate symbol '_link_test123' in:
	    ../../src/box/libbox.a(box.cc.o)
	    ../../src/box/libbox.a(expr.c.o)
	ld: 1 duplicate symbol for architecture x86_64

It means if we ever have another expr, there will be a conflict. Does not
matter if they intersect in code. We will get a compile error even on the
struct name duplicate I think. But not sure.

See 2 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index eb169aeb8..e832984c3 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
>  	}
>  }
>  
> +struct Expr *
> +expr_new_variable(struct Parse *parse, const struct Token *spec,
> +		  const struct Token *id)
> +{
> +	assert(spec != NULL && spec->n == 1);
> +	uint32_t len = 1;
> +	if (parse->parse_only) {
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
> +			 parse->line_count, parse->line_pos,
> +			 "bindings are not allowed in DDL");
> +		parse->is_aborted = true;
> +		return NULL;
> +	}
> +	if (id != NULL) {
> +		assert(spec->z[0] != '?');
> +		if (id->z - spec->z != 1) {
> +			diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN,
> +				 parse->line_count, spec->z - parse->zTail + 1,
> +				 spec->n, spec->z);
> +			parse->is_aborted = true;
> +			return NULL;
> +		}
> +		if (spec->z[0] == '#' && sqlIsdigit(id->z[0])) {
> +			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
> +				 parse->line_count, spec->n, spec->z);
> +			parse->is_aborted = true;
> +			return NULL;
> +		}
> +		len += id->n;
> +	}
> +	struct Expr *expr = sql_expr_new_empty(parse->db, TK_VARIABLE, len + 1);
> +	expr->type = FIELD_TYPE_BOOLEAN;

1. It will crash in case allocation fails and expr == NULL. Although maybe
it is not important if we plan to add panic() on malloc failure in SQL. The
same way as it already works in xmalloc().

> +	expr->flags = EP_Leaf;
> +	expr->iAgg = -1;

2. iAgg -1 is already set in sql_expr_new_empty(). nHeight too. And
then the assert about SQL_MAX_EXPR_DEPTH > 0 is not needed either.

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

* Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values
  2021-12-02  8:38         ` Mergen Imeev via Tarantool-patches
@ 2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
  2021-12-13  7:42             ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-12-09  0:31 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Thanks for the fixes!

>>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>>>> index 74a98c550..789d8906c 100644
>>>>> --- a/src/box/sql/expr.c
>>>>> +++ b/src/box/sql/expr.c
>>>>> @@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
>>>>>  	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
>>>>>  }
>>>>>  
>>>>> +static void
>>>>> +expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
>>>>
>>>> 1. I thought the policy was that we name functions, generating VDBE code,
>>>> using 'emit' suffix. For instance, `vdbe_emit_map()` or `sql_emit_map()`.
>>>> Don't know about prefix though. I see both vdbe_ and sql_ are used.
>>>>
>>> This is usually true, but this function is actually part of sqlExprCodeTarget().
>>> I believe these functions were created to make sqlExprCodeTarget() more
>>> readable. All such functions are named sqlExprCode*(), code*() or
>>> expr_code _*(), for example: sqlExprCodeGetColumn(), codeReal(),
>>> expr_code_int().
>>>
>>> Since all these functions are static, I think we should drop "expr_" prefix for
>>> them. Not in this patch, though.
>>
>> If functions take Expr as an argument like these do, they could be
>> considered methods of Expr. In that case dropping the expr_ prefix would
>> violate our naming convention. It is not about static or global here.
>>
>> As an alternative they could be considered as methods of Parse, but
>> then they would need to have parse_ prefix.
>>
>> For 'code' vs 'emit' - 'code' is fine by me as long as it is static. But
>> if it goes public, then either 'code' or 'emit' must be chosen as one
>> correct suffix. Not a mix.
>>
> After some thought, I think you are right. However, I would suggest removing the
> parser and vdbe from these functions and converting them to proper struct expr
> methods. This way we can make these functions return a value (most likely as an
> "out" argument). For example expr_code_dec() should give us DECIMAL. In this
> case we can make some improvements, for example we can remove "is_neg" from
> expr_code_int() and turn it into expr_code_uint(), since we know that this '-'
> sign will be specified as another expr. Also, since these will be valid expr
> methods, we can drop "static" from their definition. We then should name them
> accordingly, for  example "expr_code_dec" may be named "expr_to_dec".

AFAIU, their value is not only in converting the value, but also in doing
the sqlVdbeAddOp action. You will need to duplicate this call in all places
where expr_to_dec() would be used. The aspiration for refactoring this code
is righteous anyway though.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 55e494332..86de3f98a 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1438,6 +1438,26 @@ case OP_Array: {
>  	break;
>  }
>  
> +/**
> + * Opcode: Map P1 P2 P3 * *
> + * Synopsis: r[P2] = map(P3@P1)
> + *
> + * Construct an MAP value from P1 registers starting at reg(P3).
> + */
> +case OP_Map: {
> +	pOut = &aMem[pOp->p2];
> +
> +	uint32_t size;
> +	struct region *region = &fiber()->gc;
> +	size_t svp = region_used(region);
> +	char *val = mem_encode_map(&aMem[pOp->p3], pOp->p1, &size, region);
> +	if (val == NULL || mem_copy_map(pOut, val, size) != 0) {
> +		region_truncate(region, svp);
> +		goto abort_due_to_error;
> +	}

You should probably truncate the region regardless of the result. Otherwise
in case of success you will leak the region inside of the query bit by bit
while SELECT works:

	box.execute('CREATE TABLE test (id INTEGER PRIMARY KEY)')
	box.execute('INSERT INTO test VALUES (1), (2), (3)')
	box.execute('SELECT {id: id} FROM test')

Here you will do OP_Map 3 times, all will leave some region memory leaked
every time. It is freed in the end of execution probably, but it might do
some big usage while the request is in progress when the row count is much
bigger than 3.

Btw, worth adding a multirow test. All current map tests select a single row.

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

* Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
  2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-12-13  7:34             ` Mergen Imeev via Tarantool-patches
  2021-12-13 21:47               ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-12-13  7:34 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

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

On Thu, Dec 09, 2021 at 01:31:34AM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> >>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> >>>>> index eb169aeb8..74a98c550 100644
> >>>>> --- a/src/box/sql/expr.c
> >>>>> +++ b/src/box/sql/expr.c
> >>>>> @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
> >>>>>  	}
> >>>>>  }
> >>>>>  
> >>>>> +struct Expr *
> >>>>> +expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
> >>>>
> >>>> 1. You might want to call it expr_new_variable(). Or sql_expr_new_variable().
> >>>> To be consistent with our naming policy for constructors allocating memory
> >>>> and for consistency with with sql_expr_new_column(), sql_expr_new(),
> >>>> sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon().
> >>>>
> >>> Thank you! I renamed it to expr_new_variable(). I believe we should drop 'sql_'
> >>> prefix for functions that only accessible in SQL.
> >>
> >> It would work for static functions. But if a function is visible in other
> >> modules as a symbol, then you would get a conflict during linking if we
> >> ever introduce another 'struct expr' somewhere. Even if they do not interest
> >> anywhere in the code. However I don't mind leaving it as is. It can be fixed
> >> later if ever needed.
> >>
> > I agree. However, I think we need to rework all the places where BOX uses
> > internal SQL functions and structures. In this case, the struct expr should
> > never be available in the BOX, so there should be no conflicts.
> 
> It is a misunderstanding. It does not matter if you use a function in box or
> not. If it is not static and is defined in 2 places - you will get a conflict
> during link stage. Try to add a function with the same name to any file in
> sql and to any file in box. Something like
> 
> 	void
> 	link_test123(void)
> 	{
> 		printf("in link test\n");
> 	}
> 
> (in a .cc file you would need to add 'extern "C"' for it). It will raise an
> error during build. I added it to expr.c and to box.cc (with 'extern "C"'):
> 
> 	duplicate symbol '_link_test123' in:
> 	    ../../src/box/libbox.a(box.cc.o)
> 	    ../../src/box/libbox.a(expr.c.o)
> 	ld: 1 duplicate symbol for architecture x86_64
> 
Got it, thanks for the explanation. It might be better to rename "struct Expr"
to "struct sql_expr", in which case we will naturally use the sql_expr_ * prefix
for such functions. How do you think?

> It means if we ever have another expr, there will be a conflict. Does not
> matter if they intersect in code. We will get a compile error even on the
> struct name duplicate I think. But not sure.
> 
> See 2 comments below.
> 
> > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> > index eb169aeb8..e832984c3 100644
> > --- a/src/box/sql/expr.c
> > +++ b/src/box/sql/expr.c
> > @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
> >  	}
> >  }
> >  
> > +struct Expr *
> > +expr_new_variable(struct Parse *parse, const struct Token *spec,
> > +		  const struct Token *id)
> > +{
> > +	assert(spec != NULL && spec->n == 1);
> > +	uint32_t len = 1;
> > +	if (parse->parse_only) {
> > +		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
> > +			 parse->line_count, parse->line_pos,
> > +			 "bindings are not allowed in DDL");
> > +		parse->is_aborted = true;
> > +		return NULL;
> > +	}
> > +	if (id != NULL) {
> > +		assert(spec->z[0] != '?');
> > +		if (id->z - spec->z != 1) {
> > +			diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN,
> > +				 parse->line_count, spec->z - parse->zTail + 1,
> > +				 spec->n, spec->z);
> > +			parse->is_aborted = true;
> > +			return NULL;
> > +		}
> > +		if (spec->z[0] == '#' && sqlIsdigit(id->z[0])) {
> > +			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
> > +				 parse->line_count, spec->n, spec->z);
> > +			parse->is_aborted = true;
> > +			return NULL;
> > +		}
> > +		len += id->n;
> > +	}
> > +	struct Expr *expr = sql_expr_new_empty(parse->db, TK_VARIABLE, len + 1);
> > +	expr->type = FIELD_TYPE_BOOLEAN;
> 
> 1. It will crash in case allocation fails and expr == NULL. Although maybe
> it is not important if we plan to add panic() on malloc failure in SQL. The
> same way as it already works in xmalloc().
> 
True, fixed.

> > +	expr->flags = EP_Leaf;
> > +	expr->iAgg = -1;
> 
> 2. iAgg -1 is already set in sql_expr_new_empty(). nHeight too. And
> then the assert about SQL_MAX_EXPR_DEPTH > 0 is not needed either.
Thanks, dropped.


Diff:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e832984c3..8df314b17 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1345,16 +1345,15 @@ expr_new_variable(struct Parse *parse, const struct Token *spec,
 		len += id->n;
 	}
 	struct Expr *expr = sql_expr_new_empty(parse->db, TK_VARIABLE, len + 1);
+	if (expr == NULL)
+		return NULL;
 	expr->type = FIELD_TYPE_BOOLEAN;
 	expr->flags = EP_Leaf;
-	expr->iAgg = -1;
 	expr->u.zToken = (char *)(expr + 1);
 	expr->u.zToken[0] = spec->z[0];
 	if (id != NULL)
 		memcpy(expr->u.zToken + 1, id->z, id->n);
 	expr->u.zToken[len] = '\0';
-	assert(SQL_MAX_EXPR_DEPTH > 0);
-	expr->nHeight = 1;
 
 	sqlExprAssignVarNumber(parse, expr, len);
 	return expr;

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

* Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values
  2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-12-13  7:42             ` Mergen Imeev via Tarantool-patches
  2021-12-13 21:48               ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 17+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-12-13  7:42 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

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

On Thu, Dec 09, 2021 at 01:31:39AM +0100, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> >>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> >>>>> index 74a98c550..789d8906c 100644
> >>>>> --- a/src/box/sql/expr.c
> >>>>> +++ b/src/box/sql/expr.c
> >>>>> @@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
> >>>>>  	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
> >>>>>  }
> >>>>>  
> >>>>> +static void
> >>>>> +expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
> >>>>
> >>>> 1. I thought the policy was that we name functions, generating VDBE code,
> >>>> using 'emit' suffix. For instance, `vdbe_emit_map()` or `sql_emit_map()`.
> >>>> Don't know about prefix though. I see both vdbe_ and sql_ are used.
> >>>>
> >>> This is usually true, but this function is actually part of sqlExprCodeTarget().
> >>> I believe these functions were created to make sqlExprCodeTarget() more
> >>> readable. All such functions are named sqlExprCode*(), code*() or
> >>> expr_code _*(), for example: sqlExprCodeGetColumn(), codeReal(),
> >>> expr_code_int().
> >>>
> >>> Since all these functions are static, I think we should drop "expr_" prefix for
> >>> them. Not in this patch, though.
> >>
> >> If functions take Expr as an argument like these do, they could be
> >> considered methods of Expr. In that case dropping the expr_ prefix would
> >> violate our naming convention. It is not about static or global here.
> >>
> >> As an alternative they could be considered as methods of Parse, but
> >> then they would need to have parse_ prefix.
> >>
> >> For 'code' vs 'emit' - 'code' is fine by me as long as it is static. But
> >> if it goes public, then either 'code' or 'emit' must be chosen as one
> >> correct suffix. Not a mix.
> >>
> > After some thought, I think you are right. However, I would suggest removing the
> > parser and vdbe from these functions and converting them to proper struct expr
> > methods. This way we can make these functions return a value (most likely as an
> > "out" argument). For example expr_code_dec() should give us DECIMAL. In this
> > case we can make some improvements, for example we can remove "is_neg" from
> > expr_code_int() and turn it into expr_code_uint(), since we know that this '-'
> > sign will be specified as another expr. Also, since these will be valid expr
> > methods, we can drop "static" from their definition. We then should name them
> > accordingly, for  example "expr_code_dec" may be named "expr_to_dec".
> 
> AFAIU, their value is not only in converting the value, but also in doing
> the sqlVdbeAddOp action. You will need to duplicate this call in all places
> where expr_to_dec() would be used. The aspiration for refactoring this code
> is righteous anyway though.
> 
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 55e494332..86de3f98a 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -1438,6 +1438,26 @@ case OP_Array: {
> >  	break;
> >  }
> >  
> > +/**
> > + * Opcode: Map P1 P2 P3 * *
> > + * Synopsis: r[P2] = map(P3@P1)
> > + *
> > + * Construct an MAP value from P1 registers starting at reg(P3).
> > + */
> > +case OP_Map: {
> > +	pOut = &aMem[pOp->p2];
> > +
> > +	uint32_t size;
> > +	struct region *region = &fiber()->gc;
> > +	size_t svp = region_used(region);
> > +	char *val = mem_encode_map(&aMem[pOp->p3], pOp->p1, &size, region);
> > +	if (val == NULL || mem_copy_map(pOut, val, size) != 0) {
> > +		region_truncate(region, svp);
> > +		goto abort_due_to_error;
> > +	}
> 
> You should probably truncate the region regardless of the result. Otherwise
> in case of success you will leak the region inside of the query bit by bit
> while SELECT works:
> 
> 	box.execute('CREATE TABLE test (id INTEGER PRIMARY KEY)')
> 	box.execute('INSERT INTO test VALUES (1), (2), (3)')
> 	box.execute('SELECT {id: id} FROM test')
> 
> Here you will do OP_Map 3 times, all will leave some region memory leaked
> every time. It is freed in the end of execution probably, but it might do
> some big usage while the request is in progress when the row count is much
> bigger than 3.
> 
Thank you! Fixed.

> Btw, worth adding a multirow test. All current map tests select a single row.
There are several tests where the number of selected rows is more than one, for
example map-3 or map-4, but the number of rows is still quite low. I added
another test that selects 1000 rows with maps.



Diff:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 6824f11bf..33207db6b 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1456,6 +1456,7 @@ case OP_Map: {
 		region_truncate(region, svp);
 		goto abort_due_to_error;
 	}
+	region_truncate(region, svp);
 	break;
 }
 
diff --git a/test/sql-tap/map.test.lua b/test/sql-tap/map.test.lua
index 7791ca779..c51880cfd 100755
--- a/test/sql-tap/map.test.lua
+++ b/test/sql-tap/map.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(131)
+test:plan(132)
 
 box.schema.func.create('M1', {
     language = 'Lua',
@@ -1165,6 +1165,18 @@ test:do_test(
         "[{5: 1}]"
     })
 
+-- Make sure that multiple row with maps can be selected properly.
+local maps = {{a1 = 1}}
+local strs = "({'a1': 1})"
+for i = 2, 1000 do maps[i] = {['a'..tostring(i)] = i} end
+for i = 2, 1000 do strs = strs .. string.format(", ({'a%d': %d})", i, i) end
+test:do_execsql_test(
+    "map-15",
+    [[
+        SELECT * FROM (VALUES]]..strs..[[);
+    ]], maps)
+
+
 box.execute([[DROP TABLE t1;]])
 box.execute([[DROP TABLE t;]])
 

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

* Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
  2021-12-13  7:34             ` Mergen Imeev via Tarantool-patches
@ 2021-12-13 21:47               ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 17+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-12-13 21:47 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi! Thanks for the fixes!

>>>>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>>>>>> index eb169aeb8..74a98c550 100644
>>>>>>> --- a/src/box/sql/expr.c
>>>>>>> +++ b/src/box/sql/expr.c
>>>>>>> @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
>>>>>>>  	}
>>>>>>>  }
>>>>>>>  
>>>>>>> +struct Expr *
>>>>>>> +expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
>>>>>>
>>>>>> 1. You might want to call it expr_new_variable(). Or sql_expr_new_variable().
>>>>>> To be consistent with our naming policy for constructors allocating memory
>>>>>> and for consistency with with sql_expr_new_column(), sql_expr_new(),
>>>>>> sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon().
>>>>>>
>>>>> Thank you! I renamed it to expr_new_variable(). I believe we should drop 'sql_'
>>>>> prefix for functions that only accessible in SQL.
>>>>
>>>> It would work for static functions. But if a function is visible in other
>>>> modules as a symbol, then you would get a conflict during linking if we
>>>> ever introduce another 'struct expr' somewhere. Even if they do not interest
>>>> anywhere in the code. However I don't mind leaving it as is. It can be fixed
>>>> later if ever needed.
>>>>
>>> I agree. However, I think we need to rework all the places where BOX uses
>>> internal SQL functions and structures. In this case, the struct expr should
>>> never be available in the BOX, so there should be no conflicts.
>>
>> It is a misunderstanding. It does not matter if you use a function in box or
>> not. If it is not static and is defined in 2 places - you will get a conflict
>> during link stage. Try to add a function with the same name to any file in
>> sql and to any file in box. Something like
>>
>> 	void
>> 	link_test123(void)
>> 	{
>> 		printf("in link test\n");
>> 	}
>>
>> (in a .cc file you would need to add 'extern "C"' for it). It will raise an
>> error during build. I added it to expr.c and to box.cc (with 'extern "C"'):
>>
>> 	duplicate symbol '_link_test123' in:
>> 	    ../../src/box/libbox.a(box.cc.o)
>> 	    ../../src/box/libbox.a(expr.c.o)
>> 	ld: 1 duplicate symbol for architecture x86_64
>>
> Got it, thanks for the explanation. It might be better to rename "struct Expr"
> to "struct sql_expr", in which case we will naturally use the sql_expr_ * prefix
> for such functions. How do you think?

Sounds good to me.

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

* Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values
  2021-12-13  7:42             ` Mergen Imeev via Tarantool-patches
@ 2021-12-13 21:48               ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 17+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-12-13 21:48 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Thanks for the fixes!

>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>>> index 55e494332..86de3f98a 100644
>>> --- a/src/box/sql/vdbe.c
>>> +++ b/src/box/sql/vdbe.c
>>> @@ -1438,6 +1438,26 @@ case OP_Array: {
>>>  	break;
>>>  }
>>>  
>>> +/**
>>> + * Opcode: Map P1 P2 P3 * *
>>> + * Synopsis: r[P2] = map(P3@P1)
>>> + *
>>> + * Construct an MAP value from P1 registers starting at reg(P3).
>>> + */
>>> +case OP_Map: {
>>> +	pOut = &aMem[pOp->p2];
>>> +
>>> +	uint32_t size;
>>> +	struct region *region = &fiber()->gc;
>>> +	size_t svp = region_used(region);
>>> +	char *val = mem_encode_map(&aMem[pOp->p3], pOp->p1, &size, region);
>>> +	if (val == NULL || mem_copy_map(pOut, val, size) != 0) {
>>> +		region_truncate(region, svp);
>>> +		goto abort_due_to_error;
>>> +	}
>>
>> You should probably truncate the region regardless of the result. Otherwise
>> in case of success you will leak the region inside of the query bit by bit
>> while SELECT works:
>>
>> 	box.execute('CREATE TABLE test (id INTEGER PRIMARY KEY)')
>> 	box.execute('INSERT INTO test VALUES (1), (2), (3)')
>> 	box.execute('SELECT {id: id} FROM test')
>>
>> Here you will do OP_Map 3 times, all will leave some region memory leaked
>> every time. It is freed in the end of execution probably, but it might do
>> some big usage while the request is in progress when the row count is much
>> bigger than 3.
>>
> Thank you! Fixed.
> 
>> Btw, worth adding a multirow test. All current map tests select a single row.
> There are several tests where the number of selected rows is more than one, for
> example map-3 or map-4, but the number of rows is still quite low. I added
> another test that selects 1000 rows with maps.

I meant this case when you select not just already stored values, but
construct a map from the select values. For instance, not just

	SELECT col FROM t;

where 'col' is map, but rather

	SELECT {key: value} FROM t;

where 'key' and 'value' are columns in T and row count > 0.
In your new test you select '*', but I wanted to test what
happens when columns are used inside of map syntax + row count > 0.

Anyway it works, so the patchset LGTM.

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

end of thread, other threads:[~2021-12-13 21:48 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 14:08 [Tarantool-patches] [PATCH v1 0/2] Introduce syntax for MAP values is SQL Mergen Imeev via Tarantool-patches
2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names Mergen Imeev via Tarantool-patches
2021-11-20  0:45   ` Vladislav Shpilevoy via Tarantool-patches
2021-11-25  8:33     ` Mergen Imeev via Tarantool-patches
2021-11-30 22:02       ` Vladislav Shpilevoy via Tarantool-patches
2021-12-02  8:32         ` Mergen Imeev via Tarantool-patches
2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
2021-12-13  7:34             ` Mergen Imeev via Tarantool-patches
2021-12-13 21:47               ` Vladislav Shpilevoy via Tarantool-patches
2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values Mergen Imeev via Tarantool-patches
2021-11-20  0:46   ` Vladislav Shpilevoy via Tarantool-patches
2021-11-25  8:55     ` Mergen Imeev via Tarantool-patches
2021-11-30 22:04       ` Vladislav Shpilevoy via Tarantool-patches
2021-12-02  8:38         ` Mergen Imeev via Tarantool-patches
2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
2021-12-13  7:42             ` Mergen Imeev via Tarantool-patches
2021-12-13 21:48               ` Vladislav Shpilevoy via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox