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 v2 2/3] sql: make mem_is_bin() to check only for VARBINARY Date: Thu, 29 Apr 2021 23:05:19 +0200 [thread overview] Message-ID: <e58c3fcf-1ded-f92f-5a37-1628dac593e8@tarantool.org> (raw) In-Reply-To: <73769b1c3a358ebdbdb603592bda2353c0e6b800.1619542456.git.imeevma@gmail.com> Thanks for working on this! See 11 comments below! On 27.04.2021 18:55, Mergen Imeev via Tarantool-patches wrote: > After this patch, the mem_is_bin() function will return 'true' only if > the value that the MEM contains is of type VARBINARY. This patch also > adds the mem_is_bin_ext() function, which is used to check if a MEM > contains value of type VARBINARY or value of types that are currently > considered VARBINARY extensions - MAP and ARRAY. > > Part of #4906 > --- > src/box/sql/func.c | 10 +++++----- > src/box/sql/mem.h | 8 +++++++- > src/box/sql/vdbe.c | 16 ++++++++-------- > 3 files changed, 20 insertions(+), 14 deletions(-) > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index d282b2cea..bed7e8488 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -553,7 +553,7 @@ roundFunc(sql_context * context, int argc, sql_value ** argv) > } > if (mem_is_null(argv[0])) > return; > - if (mem_is_bin(argv[0])) { > + if (mem_is_bin_ext(argv[0])) { 1. The name is quite confusing. I spent some time thinking about a better one, but can't find any. Maybe because such a function looks very strange in its purpose, as well as some of its usages. Maybe better add a function mem_is_nested() to check for MP_ARRAY and MP_MAP. Or mem_is_scalar(), which checks it is not MP_ARRAY and not MP_MAP and you call it with negation. Or add special checks for array and map. mem_is_bin_ext() would be either mem_is_bin() || mem_is_nested() or mem_is_bin() || !mem_is_scalar() or mem_is_bin() || mem_is_array() || mem_is_map() Then you would see something strange below. Why in ROUND() we check for types we do not accept instead of checking the types we do accept? I mean, why not if (!mem_is_number() && !mem_is_str()) return error; instead of if (mem_is_bin_ext()) return error; What will happen when you will add UUID, date? Will you change this on each new type like that?: if (mem_is_bin_ext() || mem_is_uuid() || mem_is_date() || ...) return error; IMO would be easier to maintain a list of supported types than a list of not supported types. Worth changing now? > diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > mem_str(argv[0]), "numeric"); > context->is_aborted = true; > @@ -613,7 +613,7 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv) \ > const char *z2; \ > int n; \ > UNUSED_PARAMETER(argc); \ > - if (mem_is_bin(argv[0])) { \ > + if (mem_is_bin_ext(argv[0])) { \ 2. Ditto. > diag_set(ClientError, ER_INCONSISTENT_TYPES, "text", \ > "varbinary"); \ > context->is_aborted = true; \ > @@ -694,7 +694,7 @@ randomBlob(sql_context * context, int argc, sql_value ** argv) > unsigned char *p; > assert(argc == 1); > UNUSED_PARAMETER(argc); > - if (mem_is_bin(argv[0])) { > + if (mem_is_bin_ext(argv[0])) { 3. Ditto. > diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > mem_str(argv[0]), "numeric"); > context->is_aborted = true; > @@ -1455,7 +1455,7 @@ trim_func_one_arg(struct sql_context *context, sql_value *arg) > const unsigned char *default_trim; > if (mem_is_null(arg)) > return; > - if (mem_is_bin(arg)) > + if (mem_is_bin_ext(arg)) 4. TBH, I would ban MP_ARRAY and MP_MAP from TRIM(). It makes 0 sense for them. I am not sure it is even tested. So we are not talking about a notable 'behaviour change' really. > default_trim = (const unsigned char *) "\0"; > else > default_trim = (const unsigned char *) " "; > @@ -1584,7 +1584,7 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv) > 1, 2, 6, 2, 3, 0, 1, 0, 2, 0, 2, 0, 0, 0, 0, 0, > }; > assert(argc == 1); > - if (mem_is_bin(argv[0])) { > + if (mem_is_bin_ext(argv[0])) { 5. Ditto. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 2308587e7..bedfa87af 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -1634,9 +1634,9 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ > "boolean"); > goto abort_due_to_error; > } > - } else if (mem_is_bin(pIn3) || mem_is_bin(pIn1)) { > + } else if (mem_is_bin_ext(pIn3) || mem_is_bin_ext(pIn1)) { > if (mem_cmp_bin(pIn3, pIn1, &res) != 0) { 6. Arrays and maps are not comparable by memcmp(). The same numbers can be encoded in MessagePack as 8 bytes and as 1 byte (you can do that legally, just not widely used). In 2 maps elements might be in different order. And obviously you can't compare map and array. Such places would draw more attention for future fixes if we wouldn't mask them behind _ext() suffix but would rather use more explicit checks. > - char *str = !mem_is_bin(pIn3) ? > + char *str = !mem_is_bin_ext(pIn3) ? > mem_type_to_str(pIn3) : > mem_type_to_str(pIn1); > diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str, > @@ -2991,7 +2991,7 @@ case OP_Found: { /* jump, in3 */ > } else { > pFree = pIdxKey = sqlVdbeAllocUnpackedRecord(db, pC->key_def); > if (pIdxKey==0) goto no_mem; > - assert(mem_is_bin(pIn3)); > + assert(mem_is_bin_ext(pIn3)); 7. This would be mem_is_array() AFAIU. Because it is a tuple, right? > (void)ExpandBlob(pIn3); > sqlVdbeRecordUnpackMsgpack(pC->key_def, > pIn3->z, pIdxKey); > @@ -3253,7 +3253,7 @@ case OP_SorterData: { > assert(isSorter(pC)); > if (sqlVdbeSorterRowkey(pC, pOut) != 0) > goto abort_due_to_error; > - assert(mem_is_bin(pOut)); > + assert(mem_is_bin_ext(pOut)); 8. Ditto. > assert(pOp->p1>=0 && pOp->p1<p->nCursor); > p->apCsr[pOp->p3]->cacheStatus = CACHE_STALE; > break; > @@ -3616,7 +3616,7 @@ case OP_SorterInsert: { /* in2 */ > assert(cursor != NULL); > assert(isSorter(cursor)); > pIn2 = &aMem[pOp->p2]; > - assert(mem_is_bin(pIn2)); > + assert(mem_is_bin_ext(pIn2)); 9. Ditto. > if (ExpandBlob(pIn2) != 0 || > sqlVdbeSorterWrite(cursor, pIn2) != 0) > goto abort_due_to_error; > @@ -3650,7 +3650,7 @@ case OP_SorterInsert: { /* in2 */ > case OP_IdxReplace: > case OP_IdxInsert: { > pIn2 = &aMem[pOp->p1]; > - assert(mem_is_bin(pIn2)); > + assert(mem_is_bin_ext(pIn2)); 10. Ditto. > if (ExpandBlob(pIn2) != 0) > goto abort_due_to_error; > struct space *space; > @@ -3741,10 +3741,10 @@ case OP_Update: { > assert(pOp->p4type == P4_SPACEPTR); > > struct Mem *key_mem = &aMem[pOp->p2]; > - assert(mem_is_bin(key_mem)); > + assert(mem_is_bin_ext(key_mem)); 11. Ditto here and below. > struct Mem *upd_fields_mem = &aMem[pOp->p3]; > - assert(mem_is_bin(upd_fields_mem)); > + assert(mem_is_bin_ext(upd_fields_mem)); > uint32_t *upd_fields = (uint32_t *)upd_fields_mem->z; > uint32_t upd_fields_cnt = upd_fields_mem->n / sizeof(uint32_t);
next prev parent reply other threads:[~2021-04-29 21:05 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <cover.1619542456.git.imeevma@gmail.com> 2021-04-27 16:55 ` [Tarantool-patches] [PATCH v2 1/3] sql: initialize MEM used in aggregate functions Mergen Imeev via Tarantool-patches 2021-04-29 21:02 ` Vladislav Shpilevoy via Tarantool-patches 2021-05-17 11:38 ` Mergen Imeev via Tarantool-patches 2021-04-27 16:55 ` [Tarantool-patches] [PATCH v2 2/3] sql: make mem_is_bin() to check only for VARBINARY Mergen Imeev via Tarantool-patches 2021-04-29 21:05 ` Vladislav Shpilevoy via Tarantool-patches [this message] 2021-05-17 12:05 ` 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=e58c3fcf-1ded-f92f-5a37-1628dac593e8@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imeevma@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 2/3] sql: make mem_is_bin() to check only for VARBINARY' \ /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