From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values Date: Sat, 20 Nov 2021 01:46:57 +0100 [thread overview] Message-ID: <662f6b87-f085-ab10-53b8-d087d9598b19@tarantool.org> (raw) In-Reply-To: <4ecfb3439688bef76c96270624410dee8822176f.1637244389.git.imeevma@gmail.com> 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 ':'.
next prev parent reply other threads:[~2021-11-20 0:47 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 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
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=662f6b87-f085-ab10-53b8-d087d9598b19@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imeevma@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values' \ /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