From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Mergen Imeev <imeevma@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: introduce operator [] Date: Fri, 3 Dec 2021 00:43:55 +0100 [thread overview] Message-ID: <6e45c47c-fbf9-45eb-3680-5ee8250c51e5@tarantool.org> (raw) In-Reply-To: <20211130152030.GA101311@tarantool.org> Thanks for the patch! >> 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. But for implicit cast violations we raise a lot of errors. >>>> + 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. I would just forget about it. Comparing arrays and maps might be possible for == and !=, but it is not possible for other comparison operators. Also comparing maps for == would be very expensive because order of fields in 2 equal maps can be different. Hence you have N^2 complexity. See 4 comments below. Nothing serious. 1. Lets add a changelog file. > 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) 2. This might be nitpicking but when you use an expression in the loop condition, you risk to force is recalculation on each iteration. Because when mp_next(data) is called, compiler can't be sure data != &key. So it will recalculate key->u.u - 1 on the next iteration. To avoid that it usually helps to save the value somewhere. For instance: for (uint32_t i = 0, end = key->u.u - 1; i < end; ++i) mp_next(data); or here it can be even simpler: for (uint32_t i = 1, end = key->u.u; i < end; ++i) mp_next(data); Up to you. > 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 > @@ -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); 3. It might be better to introduce a function sql_expr_list_prepend. Otherwise it is very counter-intuitive that the first expression is on the last place, while the others are in correct places. Or add a comment in expr_code_getitem(). Or save the first expression into expr->pLeft/pRight instead of the index list. > + 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) { 4. In SQL NULL != anything else. But I managed to achieve this: box.cfg{} _ = box.schema.create_space('TEST', {format = {{name = 'F1', type = 'unsigned'}, {name = 'F2', type = 'map'}}}):create_index('pk') box.space.TEST:replace{1, {[box.NULL] = 1}} tarantool> box.execute('SELECT f2[NULL] FROM test') --- - metadata: - name: COLUMN_1 type: any rows: - [1] ... Here NULL index = NULL key. Maybe it is correct. But it is worth discussing with somebody to be sure. I can't tell how would we get NULL keys if NULL = NULL wouldn't work here TBH. IS NULL won't help.
next prev parent reply other threads:[~2021-12-02 23:43 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 2021-12-02 23:43 ` Vladislav Shpilevoy via Tarantool-patches [this message] 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=6e45c47c-fbf9-45eb-3680-5ee8250c51e5@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