Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: introduce operator []
Date: Tue, 30 Nov 2021 18:20:30 +0300	[thread overview]
Message-ID: <20211130152030.GA101311@tarantool.org> (raw)
In-Reply-To: <684c473b-33ff-11e9-b6b2-0362cc24190f@tarantool.org>

Hi! Thank you for the review! I reworked the patch a bit. Also, after a
discussion we decided to change syntax and behavior of the operation.
My answers, diff and new patch below.

On Sun, Nov 21, 2021 at 04:53:54PM +0100, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 8 comments below.
> 
> >> @TarantoolBot document
> >> Title: Operator [] in SQL
> >>
> >> Operator [] allows to get elements of MAP, ARRAY and ANY values.
> >>
> >> Rules for operator []:
> >> 1) operator applied to the value to the left of `[` ("left-value");
> >> 2) if left-value is not MAP, ARRAY or ANY, an error is thrown;
> >> 3) if there is no values between `[` and `]`, left-value is returned as
> >>    the result;
> 
> 1. Why isn't it an error? Did I understand correctly that you mean []
> with nothing inside?
> 
You understood correctly. I thought it was logical, but for now I have dropped
everything about the variable number of elements between the brackets.

> >> 4) if there is one value between `[` and `]` ("right-value"), the result
> >>    is:
> >>   a) if left-value is ANY and its primitive type is not MAP or ARRAY,
> >>      the result is NULL;
> 
> 2. Why not error? This looks a bit controversial with how hard the rules
> for explicit and implict casts are.
> 
Since there is no good way to catch an error, we try to avoid them as much as
possible.

> >>   b) if the type or primitive type of left-value is ARRAY, and
> >>      right-value is INTEGER and its value is greater than 0 and not
> >>      greater than the number of elements in ARRAY, the result will be a
> >>      value with right-value as the index, otherwise the result will be
> >>      NULL;
> 
> 3. What if the right-value is not an integer?
> 
The result will be NULL.

> >>   c) if the type or primitive type of left-value is MAP and it contains
> >>      right-value as one of its keys, the result is the value with
> >>      right-value as the key in left-value, otherwise the result is NULL;
> >> 5) if there is more than one value between `[` and `]` than
> >>    left-value[a, b, c, ...] == left-value[a][b][c]... except it will
> >>    return NULL, if any of the `[]` operators return NULL.
> >>
> >> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> >> index 789d8906c..abf47b8ef 100644
> >> --- a/src/box/sql/expr.c
> >> +++ b/src/box/sql/expr.c
> >> @@ -3461,6 +3461,30 @@ expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
> >>  	sqlVdbeAddOp3(vdbe, OP_Map, count, reg, values_reg);
> >>  }
> >>  
> >> +static void
> >> +expr_code_getitem(struct Parse *parser, struct Expr *expr, int reg)
> 
> 4. The same as for the array/map patches - should be sql_expr_emit_... or
> expr_emit - depending on what prefix will you choose to use everywhere.
> 
I still think code_*() or expr_code_*() should be used here as these functions
are static functions.

> >> +{
> >> +	struct Vdbe *vdbe = parser->pVdbe;
> >> +	struct ExprList *list = expr->x.pList;
> >> +	assert(list != NULL);
> >> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> >> index cfb88bffe..195dfde2b 100644
> >> --- a/src/box/sql/mem.c
> >> +++ b/src/box/sql/mem.c
> >> @@ -3107,6 +3107,65 @@ mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
> >>  	return map;
> >>  }
> >>  
> >> +static int
> >> +mp_getitem(const char **data, const struct Mem *key)
> >> +{
> >> +	if ((mp_typeof(**data) != MP_ARRAY && mp_typeof(**data) != MP_MAP)) {
> 
> 5. Double (()). Need only one.
> 
Fixed.

> >> +		*data = NULL;
> >> +		return 0;
> >> +	}
> >> +	const char *end = *data;
> >> +	if (mp_typeof(**data) == MP_ARRAY) {
> >> +		uint32_t size = mp_decode_array(data);
> >> +		if (!mem_is_uint(key) || key->u.u == 0 || key->u.u > size) {
> >> +			*data = NULL;
> >> +			return 0;
> >> +		}
> >> +		for (uint32_t i = 0; i < key->u.u - 1; ++i)
> >> +			mp_next(data);
> >> +		return 0;
> >> +	}
> >> +	struct Mem mem;
> >> +	mem_create(&mem);
> >> +	uint32_t size = mp_decode_map(data);
> >> +	for (uint32_t i = 0; i < size; ++i) {
> >> +		uint32_t len;
> >> +		if (mem_from_mp_ephemeral(&mem, *data, &len) != 0)
> >> +			return -1;
> 
> 6. Lets add mem_is_trivial() assertion. Since you do not destroy the mem.
> 
Thanks, added.

> >> +		*data += len;
> >> +		assert(!mem_is_map(&mem) && !mem_is_array(&mem));
> >> +		if (mem_cmp_scalar(&mem, key, NULL) == 0)
> >> +			return 0;
> >> +		mp_next(data);
> >> +	}
> >> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> >> index db7fef71a..30922b7f6 100644
> >> --- a/src/box/sql/parse.y
> >> +++ b/src/box/sql/parse.y
> >> @@ -1126,6 +1127,22 @@ expr(A) ::= CAST(X) LP expr(E) AS typedef(T) RP(Y). {
> >>    sqlExprAttachSubtrees(pParse->db, A.pExpr, E.pExpr, 0);
> >>  }
> >>  
> >> +expr(A) ::= expr(X) LB exprlist(Y) RB(E). {
> >> +  struct Expr *expr = sql_expr_new_dequoted(pParse->db, TK_GETITEM, NULL);
> 
> 7. sql_expr_new_anon().
> 
Fixed.

> >> +  if (expr == NULL) {
> >> +    sql_expr_list_delete(pParse->db, Y);
> >> +    pParse->is_aborted = true;
> >> +    return;
> >> +  }
> >> +  Y = sql_expr_list_append(pParse->db, Y, X.pExpr);
> >> +  expr->x.pList = Y;
> >> +  expr->type = FIELD_TYPE_ANY;
> >> +  sqlExprSetHeightAndFlags(pParse, expr);
> >> +  A.pExpr = expr;
> >> +  A.zStart = X.zStart;
> >> +  A.zEnd = &E.z[E.n];
> >> +}
> 
> 8. I got an assertion fail.
> 
> box.cfg{listen = 3313}
> s = box.schema.create_space('TEST', {format = {{'ID'}, {'VALUE'}}})
> _ = s:create_index('pk')
> s:replace{1, {[{key = 100}] = 200}}
> box.execute('SELECT value[1] FROM test')
> 
> Assertion failed: (!mem_is_map(&mem) && !mem_is_array(&mem)), function mp_getitem, file /Users/gerold/Work/Repositories/tarantool/src/box/sql/mem.c, line 3136.
I replaced assert with error. I did not write about this either in the rules or
in the commit message, since I am not sure if it is correct. To find a key, I
create a MEM from MP, so looking for MAP or ARRAY key is inconvenient. We can
try to go the other way - from MEM to MP. In this case, we can use memcmp() to
look up the ARRAY and MAP keys, although this would be a fairly shallow
comparison.


Diff:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 9b15e85be..8a3bab8a4 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3455,7 +3455,7 @@ expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
 		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) {
+		    type != FIELD_TYPE_UUID && expr->op != TK_GETITEM) {
 			diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Only "
 				 "integer, string and uuid can be keys in map");
 			parser->is_aborted = true;
@@ -3479,13 +3479,24 @@ expr_code_getitem(struct Parse *parser, struct Expr *expr, int reg)
 
 	enum field_type type = value->op != TK_NULL ? sql_expr_type(value) :
 			       field_type_MAX;
-	if (value->op != TK_VARIABLE && type != FIELD_TYPE_MAP &&
-	    type != FIELD_TYPE_ARRAY && type != FIELD_TYPE_ANY) {
+	if (value->op != TK_VARIABLE && value->op != TK_GETITEM &&
+	    type != FIELD_TYPE_MAP && type != FIELD_TYPE_ARRAY) {
 		diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Selecting is "
-			 "only possible from any, map and array values");
+			 "only possible from map and array values");
 		parser->is_aborted = true;
 		return;
 	}
+	for (int i = 0; i < count - 1; ++i) {
+		struct Expr *arg = list->a[i].pExpr;
+		enum field_type type = arg->op != TK_NULL ? sql_expr_type(arg) :
+				       field_type_MAX;
+		if (type == FIELD_TYPE_MAP || type == FIELD_TYPE_ARRAY) {
+			diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Map and "
+				 "array values cannot be keys");
+			parser->is_aborted = true;
+			return;
+		}
+	}
 	int reg_operands = parser->nMem + 1;
 	parser->nMem += count;
 	sqlExprCodeExprList(parser, list, reg_operands, 0, SQL_ECEL_FACTOR);
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 146fd3ffc..f52835982 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1956,7 +1956,7 @@ is_upcast(int op, enum field_type a, enum field_type b)
 static inline bool
 is_castable(int op, enum field_type a, enum field_type b)
 {
-	return is_upcast(op, a, b) || op == TK_VARIABLE ||
+	return is_upcast(op, a, b) || op == TK_VARIABLE || op == TK_GETITEM ||
 	       (sql_type_is_numeric(a) && sql_type_is_numeric(b)) ||
 	       b == FIELD_TYPE_ANY;
 }
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 042d069b4..32b0c3fb9 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -3114,7 +3114,7 @@ mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
 static int
 mp_getitem(const char **data, const struct Mem *key)
 {
-	if ((mp_typeof(**data) != MP_ARRAY && mp_typeof(**data) != MP_MAP)) {
+	if (mp_typeof(**data) != MP_ARRAY && mp_typeof(**data) != MP_MAP) {
 		*data = NULL;
 		return 0;
 	}
@@ -3136,8 +3136,12 @@ mp_getitem(const char **data, const struct Mem *key)
 		uint32_t len;
 		if (mem_from_mp_ephemeral(&mem, *data, &len) != 0)
 			return -1;
+		assert(mem_is_trivial(&mem) && !mem_is_metatype(&mem));
 		*data += len;
-		assert(!mem_is_map(&mem) && !mem_is_array(&mem));
+		if (mem_is_map(&mem) || mem_is_array(&mem)) {
+			*data = NULL;
+			return 0;
+		}
 		if (mem_cmp_scalar(&mem, key, NULL) == 0)
 			return 0;
 		mp_next(data);
@@ -3166,7 +3170,6 @@ mem_getitem(const struct Mem *mem, const struct Mem *keys, int count,
 	uint32_t len;
 	if (mem_from_mp(res, data, &len) != 0)
 		return -1;
-	res->flags |= MEM_Any;
 	return 0;
 }
 
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 2d3512f3b..2fbdea3ec 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -142,10 +142,9 @@ mem_is_any(const struct Mem *mem)
 }
 
 static inline bool
-mem_is_doc(const struct Mem *mem)
+mem_is_msgpack(const struct Mem *mem)
 {
-	return (mem->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 0 ||
-	       (mem->flags & MEM_Any) != 0;
+	return (mem->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 0;
 }
 
 static inline bool
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 805771c69..d49920133 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -155,7 +155,7 @@ cmdx ::= cmd.
 %left CONCAT.
 %left COLLATE.
 %right BITNOT.
-%left LB.
+%right LB.
 
 
 ///////////////////// Begin and end transactions. ////////////////////////////
@@ -1100,8 +1100,8 @@ expr(A) ::= CAST(X) LP expr(E) AS typedef(T) RP(Y). {
   sqlExprAttachSubtrees(pParse->db, A.pExpr, E.pExpr, 0);
 }
 
-expr(A) ::= expr(X) LB exprlist(Y) RB(E). {
-  struct Expr *expr = sql_expr_new_dequoted(pParse->db, TK_GETITEM, NULL);
+expr(A) ::= expr(X) LB getlist(Y) RB(E). {
+  struct Expr *expr = sql_expr_new_anon(pParse->db, TK_GETITEM);
   if (expr == NULL) {
     sql_expr_list_delete(pParse->db, Y);
     pParse->is_aborted = true;
@@ -1116,6 +1116,16 @@ expr(A) ::= expr(X) LB exprlist(Y) RB(E). {
   A.zEnd = &E.z[E.n];
 }
 
+getlist(A) ::= getlist(A) RB LB expr(X). {
+  A = sql_expr_list_append(pParse->db, A, X.pExpr);
+}
+getlist(A) ::= expr(X). {
+  A = sql_expr_list_append(pParse->db, NULL, X.pExpr);
+}
+
+%type getlist {ExprList *}
+%destructor getlist {sql_expr_list_delete(pParse->db, $$);}
+
 expr(A) ::= LB(X) exprlist(Y) RB(E). {
   struct Expr *expr = sql_expr_new_anon(pParse->db, TK_ARRAY);
   if (expr == NULL) {
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 6b1810508..ad8f2b3e9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1467,24 +1467,20 @@ case OP_Map: {
  */
 case OP_Getitem: {
 	int count = pOp->p1 - 1;
+	assert(count > 0);
 	struct Mem *value = &aMem[pOp->p3 + count];
-	if (!mem_is_doc(value) ) {
+	if (mem_is_null(value)) {
+		diag_set(ClientError, ER_SQL_EXECUTE, "Selecting is not "
+			 "possible from NULL");
+		goto abort_due_to_error;
+	}
+	if (mem_is_any(value) || !mem_is_msgpack(value)) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(value),
-			 "any, map or array");
+			 "map or array");
 		goto abort_due_to_error;
 	}
 
 	pOut = &aMem[pOp->p2];
-	if (count == 0) {
-		if (mem_copy(pOut, value) != 0)
-			goto abort_due_to_error;
-		break;
-	}
-	if (!mem_is_map(value) && !mem_is_array(value)) {
-		mem_set_null(pOut);
-		break;
-	}
-
 	struct Mem *keys = &aMem[pOp->p3];
 	if (mem_getitem(value, keys, count, pOut) != 0)
 		goto abort_due_to_error;
diff --git a/test/sql-tap/msgpack.test.lua b/test/sql-tap/msgpack.test.lua
index 7872426a5..a67da2ead 100755
--- a/test/sql-tap/msgpack.test.lua
+++ b/test/sql-tap/msgpack.test.lua
@@ -1,8 +1,8 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(22)
+test:plan(23)
 
--- Make sure that it is possible to get elements from MAP, ARRAY and ANY.
+-- Make sure that it is possible to get elements from MAP и ARRAY.
 test:do_execsql_test(
     "msgpack-1.1",
     [[
@@ -14,173 +14,185 @@ test:do_execsql_test(
 test:do_execsql_test(
     "msgpack-1.2",
     [[
-        SELECT CAST([123, 234, 356, 467] AS ANY)[3];
+        SELECT {'one' : 123, 3 : 'two', '123' : true}[3];
     ]], {
-        356
+        'two'
     })
 
 test:do_execsql_test(
     "msgpack-1.3",
     [[
-        SELECT CAST([123, 234, 356, 467] AS ANY)['3'];
+        SELECT {'one' : [11, 22, 33], 3 : 'two', '123' : true}['one'][2];
     ]], {
-        ""
+        22
     })
 
 test:do_execsql_test(
     "msgpack-1.4",
     [[
-        SELECT {'one' : 123, 3 : 'two', '123' : true}[3];
+        SELECT {'one' : 123, 3 : 'two', '123' : true}['three'];
     ]], {
-        'two'
+        ""
     })
 
-test:do_execsql_test(
-    "msgpack-1.5",
+--
+-- Make sure that operator [] cannot get elements from values of types other
+-- than MAP and ARRAY.
+--
+test:do_catchsql_test(
+    "msgpack-2.1",
     [[
-        SELECT {'one' : 123, 3 : 'two', '123' : true}['one'];
+        SELECT 1[1];
     ]], {
-        123
+        1, "Selecting is only possible from map and array values"
     })
 
-test:do_execsql_test(
-    "msgpack-1.6",
+test:do_catchsql_test(
+    "msgpack-2.2",
     [[
-        SELECT {'one' : 123, 3 : 'two', '123' : true}['three'];
+        SELECT -1[1];
     ]], {
-        ""
+        1, "Selecting is only possible from map and array values"
     })
 
-test:do_execsql_test(
-    "msgpack-1.7",
+test:do_catchsql_test(
+    "msgpack-2.3",
     [[
-        SELECT CAST({'one' : 123, 3 : 'two', '123' : true} AS ANY)['123'];
+        SELECT 1.1[1];
     ]], {
-        true
+        1, "Selecting is only possible from map and array values"
     })
 
-test:do_execsql_test(
-    "msgpack-1.8",
+test:do_catchsql_test(
+    "msgpack-2.4",
     [[
-        SELECT CAST(1 AS ANY)[2];
+        SELECT 1.2e0[1];
     ]], {
-        ""
+        1, "Selecting is only possible from map and array values"
     })
 
-test:do_execsql_test(
-    "msgpack-1.9",
+test:do_catchsql_test(
+    "msgpack-2.5",
     [[
-        SELECT CAST('1' AS ANY)['asd'];
+        SELECT '1'[1];
     ]], {
-        ""
+        1, "Selecting is only possible from map and array values"
     })
 
-test:do_execsql_test(
-    "msgpack-1.10",
+test:do_catchsql_test(
+    "msgpack-2.6",
     [[
-        SELECT CAST('1' AS ANY)[];
+        SELECT x'31'[1];
     ]], {
-        '1'
+        1, "Selecting is only possible from map and array values"
     })
 
---
--- Make sure that operator [] cannot get elements from values of types other
--- than MAP, ARRAY and ANY.
---
 test:do_catchsql_test(
-    "msgpack-2.1",
+    "msgpack-2.7",
     [[
-        SELECT 1[];
+        SELECT uuid()[1];
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        1, "Selecting is only possible from map and array values"
     })
 
 test:do_catchsql_test(
-    "msgpack-2.2",
+    "msgpack-2.8",
     [[
-        SELECT -1[];
+        SELECT true[1];
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        1, "Selecting is only possible from map and array values"
     })
 
 test:do_catchsql_test(
-    "msgpack-2.3",
+    "msgpack-2.9",
     [[
-        SELECT 1.1[];
+        SELECT CAST(1 AS NUMBER)[1];
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        1, "Selecting is only possible from map and array values"
     })
 
 test:do_catchsql_test(
-    "msgpack-2.4",
+    "msgpack-2.10",
     [[
-        SELECT 1.2e0[];
+        SELECT CAST('a' AS STRING)[1];
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        1, "Selecting is only possible from map and array values"
     })
 
 test:do_catchsql_test(
-    "msgpack-2.5",
+    "msgpack-2.11",
     [[
-        SELECT '1'[];
+        SELECT NULL[1];
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        1, "Selecting is only possible from map and array values"
     })
 
 test:do_catchsql_test(
-    "msgpack-2.6",
+    "msgpack-2.12",
     [[
-        SELECT x'31'[];
+        SELECT CAST(NULL AS ANY)[1];
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        1, "Selecting is only possible from map and array values"
     })
 
-test:do_catchsql_test(
-    "msgpack-2.7",
+-- Make sure that the second and the following brackets do not throw type error.
+test:do_execsql_test(
+    "msgpack-3.1",
+    [[
+        SELECT [1, 2, 3][1][2];
+    ]], {
+        ""
+    })
+
+test:do_execsql_test(
+    "msgpack-3.2",
     [[
-        SELECT uuid()[];
+        SELECT [1, 2, 3][1][2][3][4][5][6][7];
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        ""
     })
 
 test:do_catchsql_test(
-    "msgpack-2.8",
+    "msgpack-3.3",
     [[
-        SELECT true[];
+        SELECT ([1, 2, 3][1])[2];
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        1, "Type mismatch: can not convert integer(1) to map or array"
     })
 
-test:do_catchsql_test(
-    "msgpack-2.9",
+-- Make sure that type of result of [] is checked in runtime.
+test:do_execsql_test(
+    "msgpack-4.1",
     [[
-        SELECT CAST(1 AS NUMBER)[];
+        SELECT [123, 23, 1][2] + 7;
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        30
     })
 
 test:do_catchsql_test(
-    "msgpack-2.10",
+    "msgpack-4.2",
     [[
-        SELECT CAST('a' AS STRING)[];
+        SELECT ['a', 2, true][1] + 1;
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        1, "Type mismatch: can not convert string('a') to integer, decimal "..
+        "or double"
     })
 
-test:do_catchsql_test(
-    "msgpack-2.11",
+test:do_execsql_test(
+    "msgpack-4.3",
     [[
-        SELECT NULL[];
+        SELECT {[11, 22, 33][3] : 'asd'};
     ]], {
-        1, "Selecting is only possible from any, map and array values"
+        {[33] = 'asd'}
     })
 
-test:do_catchsql_test(
-    "msgpack-2.12",
+-- Built-in functions in this case will use default types of arguments.
+test:do_execsql_test(
+    "msgpack-4.4",
     [[
-        SELECT CAST(NULL AS ANY)[];
+        SELECT typeof(ABS([123, 23, 1][1]));
     ]], {
-        1, "Type mismatch: can not convert NULL to any, map or array"
+        "decimal"
     })
 
 test:finish_test()


New patch:

commit 5442c79ab86f469d2c2997416b765167c619c5fc
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Wed Nov 17 10:21:41 2021 +0300

    sql: introduce operator []
    
    This patch introduces operator [] that allows to get elements from MAP
    and ARRAY values.
    
    Closes #4762
    Closes #4763
    Part of #6251
    
    @TarantoolBot document
    Title: Operator [] in SQL
    
    Operator `[]` allows to get elements of MAP, ARRAY and ANY values.
    
    Rules for operator `[]`:
    1) operator applied to the expression to the left of `[` ("container");
    2) between `[` and `]` should be one expression ("key");
    3) if container is NULL or not MAP or ARRAY:
      a) If there is only one such operator, or if there is a sequence of []
         operators, and this is the first of them, an error is thrown;
      b) otherwise the result is NULL;
    4) if container is ARRAY, then:
      a) if key is INTEGER and its value is greater than 0 and not more than
         the number of elements in the container, the result will be the
         value in the container with key as the index;
      b) otherwise the result will be NULL;
    5) if container is MAP, then:
      a) if container contains a key as one of its keys, the result is the
         value corresponding to this key;
      b) otherwise the result will be NULL;
    
    Examples:
    ```
    tarantool> box.execute([[SELECT {'a' : 12, 3 : 34}[3];]])
    ---
    - metadata:
      - name: COLUMN_1
        type: any
      rows:
      - [34]
    ...
    ```
    
    ```
    tarantool> box.execute([[SELECT [12, [4, 5, 6], 34][2][2];]])
    ---
    - metadata:
      - name: COLUMN_1
        type: any
      rows:
      - [5]
    ...
    ```

diff --git a/extra/addopcodes.sh b/extra/addopcodes.sh
index 3f8cfdf02..51acfe38e 100755
--- a/extra/addopcodes.sh
+++ b/extra/addopcodes.sh
@@ -53,6 +53,7 @@ extras="            \
     LINEFEED        \
     SPACE           \
     ILLEGAL         \
+    GETITEM         \
 "
 
 IFS=" "
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e4c1dcff1..8a3bab8a4 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3455,7 +3455,7 @@ expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
 		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) {
+		    type != FIELD_TYPE_UUID && expr->op != TK_GETITEM) {
 			diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Only "
 				 "integer, string and uuid can be keys in map");
 			parser->is_aborted = true;
@@ -3468,6 +3468,41 @@ expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
 	sqlVdbeAddOp3(vdbe, OP_Map, count, reg, values_reg);
 }
 
+static void
+expr_code_getitem(struct Parse *parser, struct Expr *expr, int reg)
+{
+	struct Vdbe *vdbe = parser->pVdbe;
+	struct ExprList *list = expr->x.pList;
+	assert(list != NULL);
+	int count = list->nExpr;
+	struct Expr *value = list->a[count - 1].pExpr;
+
+	enum field_type type = value->op != TK_NULL ? sql_expr_type(value) :
+			       field_type_MAX;
+	if (value->op != TK_VARIABLE && value->op != TK_GETITEM &&
+	    type != FIELD_TYPE_MAP && type != FIELD_TYPE_ARRAY) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Selecting is "
+			 "only possible from map and array values");
+		parser->is_aborted = true;
+		return;
+	}
+	for (int i = 0; i < count - 1; ++i) {
+		struct Expr *arg = list->a[i].pExpr;
+		enum field_type type = arg->op != TK_NULL ? sql_expr_type(arg) :
+				       field_type_MAX;
+		if (type == FIELD_TYPE_MAP || type == FIELD_TYPE_ARRAY) {
+			diag_set(ClientError, ER_SQL_PARSER_GENERIC, "Map and "
+				 "array values cannot be keys");
+			parser->is_aborted = true;
+			return;
+		}
+	}
+	int reg_operands = parser->nMem + 1;
+	parser->nMem += count;
+	sqlExprCodeExprList(parser, list, reg_operands, 0, SQL_ECEL_FACTOR);
+	sqlVdbeAddOp3(vdbe, OP_Getitem, count, reg, reg_operands);
+}
+
 /*
  * Erase column-cache entry number i
  */
@@ -3927,6 +3962,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 		expr_code_map(pParse, pExpr, target);
 		return target;
 
+	case TK_GETITEM:
+		expr_code_getitem(pParse, pExpr, target);
+		return target;
+
 	case TK_LT:
 	case TK_LE:
 	case TK_GT:
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 146fd3ffc..f52835982 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1956,7 +1956,7 @@ is_upcast(int op, enum field_type a, enum field_type b)
 static inline bool
 is_castable(int op, enum field_type a, enum field_type b)
 {
-	return is_upcast(op, a, b) || op == TK_VARIABLE ||
+	return is_upcast(op, a, b) || op == TK_VARIABLE || op == TK_GETITEM ||
 	       (sql_type_is_numeric(a) && sql_type_is_numeric(b)) ||
 	       b == FIELD_TYPE_ANY;
 }
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 7411b8f67..32b0c3fb9 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -3111,6 +3111,68 @@ mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
 	return map;
 }
 
+static int
+mp_getitem(const char **data, const struct Mem *key)
+{
+	if (mp_typeof(**data) != MP_ARRAY && mp_typeof(**data) != MP_MAP) {
+		*data = NULL;
+		return 0;
+	}
+	const char *end = *data;
+	if (mp_typeof(**data) == MP_ARRAY) {
+		uint32_t size = mp_decode_array(data);
+		if (!mem_is_uint(key) || key->u.u == 0 || key->u.u > size) {
+			*data = NULL;
+			return 0;
+		}
+		for (uint32_t i = 0; i < key->u.u - 1; ++i)
+			mp_next(data);
+		return 0;
+	}
+	struct Mem mem;
+	mem_create(&mem);
+	uint32_t size = mp_decode_map(data);
+	for (uint32_t i = 0; i < size; ++i) {
+		uint32_t len;
+		if (mem_from_mp_ephemeral(&mem, *data, &len) != 0)
+			return -1;
+		assert(mem_is_trivial(&mem) && !mem_is_metatype(&mem));
+		*data += len;
+		if (mem_is_map(&mem) || mem_is_array(&mem)) {
+			*data = NULL;
+			return 0;
+		}
+		if (mem_cmp_scalar(&mem, key, NULL) == 0)
+			return 0;
+		mp_next(data);
+	}
+	mp_next(&end);
+	if (*data == end)
+		*data = NULL;
+	return 0;
+}
+
+int
+mem_getitem(const struct Mem *mem, const struct Mem *keys, int count,
+	    struct Mem *res)
+{
+	assert(count > 0);
+	assert(mem_is_map(mem) || mem_is_array(mem));
+	const char *data = mem->z;
+	for (int i = 0; i < count && data != NULL; ++i) {
+		if (mp_getitem(&data, &keys[i]) != 0)
+			return -1;
+	}
+	if (data == NULL) {
+		mem_set_null(res);
+		return 0;
+	}
+	uint32_t len;
+	if (mem_from_mp(res, data, &len) != 0)
+		return -1;
+	return 0;
+}
+
 /**
  * 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 7e35123ca..2fbdea3ec 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -135,6 +135,18 @@ mem_is_num(const struct Mem *mem)
 			MEM_TYPE_DEC)) != 0;
 }
 
+static inline bool
+mem_is_any(const struct Mem *mem)
+{
+	return (mem->flags & MEM_Any) != 0;
+}
+
+static inline bool
+mem_is_msgpack(const struct Mem *mem)
+{
+	return (mem->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 0;
+}
+
 static inline bool
 mem_is_metatype(const struct Mem *mem)
 {
@@ -890,3 +902,8 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
 char *
 mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
 	       struct region *region);
+
+/** Return a value from ANY, MAP, or ARRAY MEM using the MEM array as keys. */
+int
+mem_getitem(const struct Mem *mem, const struct Mem *keys, int count,
+	    struct Mem *res);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 998acadea..d49920133 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -155,6 +155,7 @@ cmdx ::= cmd.
 %left CONCAT.
 %left COLLATE.
 %right BITNOT.
+%right LB.
 
 
 ///////////////////// Begin and end transactions. ////////////////////////////
@@ -1099,6 +1100,32 @@ expr(A) ::= CAST(X) LP expr(E) AS typedef(T) RP(Y). {
   sqlExprAttachSubtrees(pParse->db, A.pExpr, E.pExpr, 0);
 }
 
+expr(A) ::= expr(X) LB getlist(Y) RB(E). {
+  struct Expr *expr = sql_expr_new_anon(pParse->db, TK_GETITEM);
+  if (expr == NULL) {
+    sql_expr_list_delete(pParse->db, Y);
+    pParse->is_aborted = true;
+    return;
+  }
+  Y = sql_expr_list_append(pParse->db, Y, X.pExpr);
+  expr->x.pList = Y;
+  expr->type = FIELD_TYPE_ANY;
+  sqlExprSetHeightAndFlags(pParse, expr);
+  A.pExpr = expr;
+  A.zStart = X.zStart;
+  A.zEnd = &E.z[E.n];
+}
+
+getlist(A) ::= getlist(A) RB LB expr(X). {
+  A = sql_expr_list_append(pParse->db, A, X.pExpr);
+}
+getlist(A) ::= expr(X). {
+  A = sql_expr_list_append(pParse->db, NULL, X.pExpr);
+}
+
+%type getlist {ExprList *}
+%destructor getlist {sql_expr_list_delete(pParse->db, $$);}
+
 expr(A) ::= LB(X) exprlist(Y) RB(E). {
   struct Expr *expr = sql_expr_new_anon(pParse->db, TK_ARRAY);
   if (expr == NULL) {
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 86de3f98a..ad8f2b3e9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1458,6 +1458,35 @@ case OP_Map: {
 	break;
 }
 
+/**
+ * Opcode: Getitem P1 P2 P3 * *
+ * Synopsis: r[P2] = value[P3@(P1 - 1)]
+ *
+ * Get an element from the value in register P3[P1 - 1] using values in
+ * registers P3, ... P3 + (P1 - 2).
+ */
+case OP_Getitem: {
+	int count = pOp->p1 - 1;
+	assert(count > 0);
+	struct Mem *value = &aMem[pOp->p3 + count];
+	if (mem_is_null(value)) {
+		diag_set(ClientError, ER_SQL_EXECUTE, "Selecting is not "
+			 "possible from NULL");
+		goto abort_due_to_error;
+	}
+	if (mem_is_any(value) || !mem_is_msgpack(value)) {
+		diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(value),
+			 "map or array");
+		goto abort_due_to_error;
+	}
+
+	pOut = &aMem[pOp->p2];
+	struct Mem *keys = &aMem[pOp->p3];
+	if (mem_getitem(value, keys, count, pOut) != 0)
+		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/engine.cfg b/test/sql-tap/engine.cfg
index 528212ab6..52ce5a1f6 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -35,6 +35,7 @@
     "built-in-functions.test.lua": {
         "memtx": {"engine": "memtx"}
     },
+    "msgpack.test.lua": {},
     "gh-6157-unnecessary-free-on-string.test.lua": {},
     "gh-6376-wrong-double-to-dec-cmp.test.lua": {},
     "gh-4077-iproto-execute-no-bind.test.lua": {},
diff --git a/test/sql-tap/msgpack.test.lua b/test/sql-tap/msgpack.test.lua
new file mode 100755
index 000000000..a67da2ead
--- /dev/null
+++ b/test/sql-tap/msgpack.test.lua
@@ -0,0 +1,198 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(23)
+
+-- Make sure that it is possible to get elements from MAP и ARRAY.
+test:do_execsql_test(
+    "msgpack-1.1",
+    [[
+        SELECT [123, 234, 356, 467][2];
+    ]], {
+        234
+    })
+
+test:do_execsql_test(
+    "msgpack-1.2",
+    [[
+        SELECT {'one' : 123, 3 : 'two', '123' : true}[3];
+    ]], {
+        'two'
+    })
+
+test:do_execsql_test(
+    "msgpack-1.3",
+    [[
+        SELECT {'one' : [11, 22, 33], 3 : 'two', '123' : true}['one'][2];
+    ]], {
+        22
+    })
+
+test:do_execsql_test(
+    "msgpack-1.4",
+    [[
+        SELECT {'one' : 123, 3 : 'two', '123' : true}['three'];
+    ]], {
+        ""
+    })
+
+--
+-- Make sure that operator [] cannot get elements from values of types other
+-- than MAP and ARRAY.
+--
+test:do_catchsql_test(
+    "msgpack-2.1",
+    [[
+        SELECT 1[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.2",
+    [[
+        SELECT -1[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.3",
+    [[
+        SELECT 1.1[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.4",
+    [[
+        SELECT 1.2e0[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.5",
+    [[
+        SELECT '1'[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.6",
+    [[
+        SELECT x'31'[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.7",
+    [[
+        SELECT uuid()[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.8",
+    [[
+        SELECT true[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.9",
+    [[
+        SELECT CAST(1 AS NUMBER)[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.10",
+    [[
+        SELECT CAST('a' AS STRING)[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.11",
+    [[
+        SELECT NULL[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+test:do_catchsql_test(
+    "msgpack-2.12",
+    [[
+        SELECT CAST(NULL AS ANY)[1];
+    ]], {
+        1, "Selecting is only possible from map and array values"
+    })
+
+-- Make sure that the second and the following brackets do not throw type error.
+test:do_execsql_test(
+    "msgpack-3.1",
+    [[
+        SELECT [1, 2, 3][1][2];
+    ]], {
+        ""
+    })
+
+test:do_execsql_test(
+    "msgpack-3.2",
+    [[
+        SELECT [1, 2, 3][1][2][3][4][5][6][7];
+    ]], {
+        ""
+    })
+
+test:do_catchsql_test(
+    "msgpack-3.3",
+    [[
+        SELECT ([1, 2, 3][1])[2];
+    ]], {
+        1, "Type mismatch: can not convert integer(1) to map or array"
+    })
+
+-- Make sure that type of result of [] is checked in runtime.
+test:do_execsql_test(
+    "msgpack-4.1",
+    [[
+        SELECT [123, 23, 1][2] + 7;
+    ]], {
+        30
+    })
+
+test:do_catchsql_test(
+    "msgpack-4.2",
+    [[
+        SELECT ['a', 2, true][1] + 1;
+    ]], {
+        1, "Type mismatch: can not convert string('a') to integer, decimal "..
+        "or double"
+    })
+
+test:do_execsql_test(
+    "msgpack-4.3",
+    [[
+        SELECT {[11, 22, 33][3] : 'asd'};
+    ]], {
+        {[33] = 'asd'}
+    })
+
+-- Built-in functions in this case will use default types of arguments.
+test:do_execsql_test(
+    "msgpack-4.4",
+    [[
+        SELECT typeof(ABS([123, 23, 1][1]));
+    ]], {
+        "decimal"
+    })
+
+test:finish_test()

  reply	other threads:[~2021-11-30 15:20 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-19 14:44 Mergen Imeev via Tarantool-patches
2021-11-19 17:25 ` Konstantin Osipov via Tarantool-patches
2021-11-21 15:53   ` Vladislav Shpilevoy via Tarantool-patches
2021-11-30 15:20     ` Mergen Imeev via Tarantool-patches [this message]
2021-12-02 23:43       ` Vladislav Shpilevoy via Tarantool-patches
2021-11-24 11:56   ` Mergen Imeev via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211130152030.GA101311@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v1 1/1] sql: introduce operator []' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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