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 2DE6270297; Fri, 3 Dec 2021 02:43:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2DE6270297 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1638488639; bh=1pxOsVby8SGk4WVSk6bYBVTIfkucvxNQdSrJoaGJ+Cs=; 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=wPR7kc0SM09BtSDtsnS4DBamZT38Nod65kUr67JQpVFGI3JBWAQ2Ht57Ycc8OoVZx 9Marrv9cwyvDZuGEBacHS1hUcqzMSr5bGxO6n2IL+7hgXc3J3L5/63BuQSrNleEIda jyYmag78mwEyY9Y4WhGnKZfbkiu/yDMuQo6H8Y0g= Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 92F5070297 for ; Fri, 3 Dec 2021 02:43:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 92F5070297 Received: by smtp43.i.mail.ru with esmtpa (envelope-from ) id 1msvk4-00019w-P4; Fri, 03 Dec 2021 02:43:57 +0300 Message-ID: <6e45c47c-fbf9-45eb-3680-5ee8250c51e5@tarantool.org> Date: Fri, 3 Dec 2021 00:43:55 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.3.2 Content-Language: en-US To: Mergen Imeev References: <20211119172502.GA136743@starling> <684c473b-33ff-11e9-b6b2-0362cc24190f@tarantool.org> <20211130152030.GA101311@tarantool.org> In-Reply-To: <20211130152030.GA101311@tarantool.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD93822B471089FF64DD66F0DCF3C6A542B99491B2D20C235A5182A05F538085040E78AFB3B1997CBF60EA5BD7C3299788F459F323A1F88E4D01B437AE24DBED0F1 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7BD76697DAE49F8B0EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637593B4F2A76947A308638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8BC0D8733E5578C6DBD528AE313A8EDF7117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAE9A1BBD95851C5BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE0AC5B80A05675ACDEA93887B71B66F2BD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3568FCE88E0C8489DAD7EC71F1DB88427C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006377AA2284B41911753EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5658073DC9F0C882793D5E37DBA2AFA771F1ED6A2DB11BAB5D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7506FE1F977233B9BB410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D346F8291983715AC66EE82F02F197091262A1336F52FCCF36888E449415CDCBD1EE7050FA0FFD971D71D7E09C32AA3244CE82389A1817A6F69416A6BC3D9CE7CAE7101BF96129E4011FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojMMFVqzO9sR0sQRipMRjVeA== X-Mailru-Sender: 1F3202E75A95DDEFB45532EB6DE299A68743D37F9FB60BCBC41F1D3242D157DB48BB63BE400E334C07784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6B0DAF586E7D11B3E67EA787935ED9F1B 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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.