From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 632DB6ECE3; Tue, 30 Nov 2021 18:20:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 632DB6ECE3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1638285634; bh=bnRFVSAHO69/cX9O7NULXMcrWm+ftfOGjLePLqJ0Sx4=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=w7448hUskfmbnRYHkxhDKsQpAK8LiQOAWqVFPkGwkWybnMPyLpUGKSzkgBpbw9dAZ g+QgZH5VAiBdYSoQFtAE6GJ5hEGypbQwVdF7HgLg/cEryYB0kUFVtALVmtbbAn1nAk 6d8McybJZpk/eHT5o4pYOLziLgONYxxqibOn9UGI= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 211E06ECE3 for ; Tue, 30 Nov 2021 18:20:33 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 211E06ECE3 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1ms4vo-0002rk-28; Tue, 30 Nov 2021 18:20:32 +0300 Date: Tue, 30 Nov 2021 18:20:30 +0300 To: Vladislav Shpilevoy Message-ID: <20211130152030.GA101311@tarantool.org> References: <20211119172502.GA136743@starling> <684c473b-33ff-11e9-b6b2-0362cc24190f@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <684c473b-33ff-11e9-b6b2-0362cc24190f@tarantool.org> X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9430AF41A71704E0BA39FACF6C5BCC35AC725BEFE9F98EE6B1313CFAB8367EF908E2BE116634AD74DF157AF01A6A9B8FBE68D2D20A124DD90B8757ED72C300332303B288D58C5C93A X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72847AA60176ABEF3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637CF58BB58AE3180708638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8BC405B9623D42CEB17E5DAFD8971B012117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B65D56369A3576CBA5089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5D805092D750B586268EEBF86B4CBD0EAA928F01AC63E8A19D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA759F66ED85EB5F25FD410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34ADE558D2B396DA7CC2CC601C44726D3069348645754B246869ED34362B5DE00AF8FED64CE4781CA81D7E09C32AA3244CF5E98DA2588D1068DE6FFEE6166F84893C6EB905E3A8056BFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojNjnaLwLelXhTt/BmYpYpOg== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D8723C1EDD0C21B4FF09C7694081BDF5C83D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: introduce operator [] X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 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()