* [Tarantool-patches] [PATCH v2 1/3] sql: initialize MEM used in aggregate functions [not found] <cover.1619542456.git.imeevma@gmail.com> @ 2021-04-27 16:55 ` Mergen Imeev via Tarantool-patches 2021-04-29 21:02 ` Vladislav Shpilevoy 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 1 sibling, 1 reply; 6+ messages in thread From: Mergen Imeev via Tarantool-patches @ 2021-04-27 16:55 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches This patch adds proper initialization for the MEM, which is used in the aggregate functions min() and max(). Part of #4906 --- src/box/sql/func.c | 21 ++++++++++----------- src/box/sql/sqlInt.h | 7 +++++++ src/box/sql/vdbeapi.c | 20 ++++++++++++++++++++ 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 9c28d5122..d282b2cea 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -1766,14 +1766,14 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) struct func_sql_builtin *func = (struct func_sql_builtin *)context->func; - pBest = (Mem *) sql_aggregate_context(context, sizeof(*pBest)); + pBest = sql_context_agg_mem(context); if (!pBest) return; if (mem_is_null(argv[0])) { - if (pBest->flags) + if (!mem_is_null(pBest)) sqlSkipAccumulatorLoad(context); - } else if (pBest->flags) { + } else if (!mem_is_null(pBest)) { int cmp; struct coll *pColl = sqlGetFuncCollSeq(context); /* @@ -1798,14 +1798,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) static void minMaxFinalize(sql_context * context) { - sql_value *pRes; - pRes = (sql_value *) sql_aggregate_context(context, 0); - if (pRes) { - if (pRes->flags) { - sql_result_value(context, pRes); - } - mem_destroy(pRes); - } + struct Mem *mem = context->pMem; + struct Mem *res; + if (!mem_is_agg(mem) || mem_get_agg(mem, (void **)&res) != 0) + return; + if (!mem_is_null(res)) + sql_result_value(context, res); + mem_destroy(res); } /* diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index b548ddad4..ef8dcd693 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -484,6 +484,13 @@ void * sql_aggregate_context(sql_context *, int nBytes); +/** + * Allocate or return the aggregate context containing struct MEM for a user + * function. A new context is allocated on the first call. Subsequent calls + * return the same context that was returned on prior calls. + */ +struct Mem * +sql_context_agg_mem(struct sql_context *context); int sql_column_count(sql_stmt * pStmt); diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 655743fb1..aaae12e41 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -394,6 +394,26 @@ sql_aggregate_context(sql_context * p, int nByte) return accum; } +struct Mem * +sql_context_agg_mem(struct sql_context *ctx) +{ + assert(ctx != NULL && ctx->func != NULL); + assert(ctx->func->def->language == FUNC_LANGUAGE_SQL_BUILTIN); + assert(ctx->func->def->aggregate == FUNC_AGGREGATE_GROUP); + struct Mem *mem; + if (!mem_is_agg(ctx->pMem)) { + if (mem_set_agg(ctx->pMem, ctx->func, sizeof(*mem)) != 0) + return NULL; + if (mem_get_agg(ctx->pMem, (void **)&mem) != 0) + return NULL; + mem_create(mem); + return mem; + } + if (mem_get_agg(ctx->pMem, (void **)&mem) != 0) + return NULL; + return mem; +} + /* * Return the number of columns in the result set for the statement pStmt. */ -- 2.25.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/3] sql: initialize MEM used in aggregate functions 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 0 siblings, 1 reply; 6+ messages in thread From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-29 21:02 UTC (permalink / raw) To: imeevma; +Cc: tarantool-patches Hi! Thanks for the patch! > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 9c28d5122..d282b2cea 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -1798,14 +1798,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) > static void > minMaxFinalize(sql_context * context) > { > - sql_value *pRes; > - pRes = (sql_value *) sql_aggregate_context(context, 0); > - if (pRes) { > - if (pRes->flags) { > - sql_result_value(context, pRes); > - } > - mem_destroy(pRes); > - } > + struct Mem *mem = context->pMem; > + struct Mem *res; > + if (!mem_is_agg(mem) || mem_get_agg(mem, (void **)&res) != 0) You don't need `is_agg` check. `get` does the same already. > + return; > + if (!mem_is_null(res)) > + sql_result_value(context, res); > + mem_destroy(res); > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/3] sql: initialize MEM used in aggregate functions 2021-04-29 21:02 ` Vladislav Shpilevoy via Tarantool-patches @ 2021-05-17 11:38 ` Mergen Imeev via Tarantool-patches 0 siblings, 0 replies; 6+ messages in thread From: Mergen Imeev via Tarantool-patches @ 2021-05-17 11:38 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hi! Thank you for the review! My answer and diff below. On Thu, Apr 29, 2021 at 11:02:06PM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > > index 9c28d5122..d282b2cea 100644 > > --- a/src/box/sql/func.c > > +++ b/src/box/sql/func.c > > @@ -1798,14 +1798,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) > > static void > > minMaxFinalize(sql_context * context) > > { > > - sql_value *pRes; > > - pRes = (sql_value *) sql_aggregate_context(context, 0); > > - if (pRes) { > > - if (pRes->flags) { > > - sql_result_value(context, pRes); > > - } > > - mem_destroy(pRes); > > - } > > + struct Mem *mem = context->pMem; > > + struct Mem *res; > > + if (!mem_is_agg(mem) || mem_get_agg(mem, (void **)&res) != 0) > > You don't need `is_agg` check. `get` does the same already. > Fixed. > > + return; > > + if (!mem_is_null(res)) > > + sql_result_value(context, res); > > + mem_destroy(res); > > } Diff: diff --git a/src/box/sql/func.c b/src/box/sql/func.c index d282b2cea..94302cc56 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -1800,7 +1800,7 @@ minMaxFinalize(sql_context * context) { struct Mem *mem = context->pMem; struct Mem *res; - if (!mem_is_agg(mem) || mem_get_agg(mem, (void **)&res) != 0) + if (mem_get_agg(mem, (void **)&res) != 0) return; if (!mem_is_null(res)) sql_result_value(context, res); ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH v2 2/3] sql: make mem_is_bin() to check only for VARBINARY [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-27 16:55 ` Mergen Imeev via Tarantool-patches 2021-04-29 21:05 ` Vladislav Shpilevoy via Tarantool-patches 1 sibling, 1 reply; 6+ messages in thread From: Mergen Imeev via Tarantool-patches @ 2021-04-27 16:55 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches 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])) { 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])) { \ 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])) { 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)) 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])) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(argv[0]), "text"); context->is_aborted = true; diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index 1db7f4deb..6fc15617d 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -184,7 +184,7 @@ mem_is_bool(const struct Mem *mem) static inline bool mem_is_bin(const struct Mem *mem) { - return (mem->flags & MEM_Blob) != 0; + return (mem->flags & MEM_Blob) != 0 && (mem->flags & MEM_Subtype) == 0; } static inline bool @@ -206,6 +206,12 @@ mem_is_array(const struct Mem *mem) mp_typeof(*mem->z) == MP_ARRAY; } +static inline bool +mem_is_bin_ext(const struct Mem *mem) +{ + return (mem->flags & MEM_Blob) != 0; +} + static inline bool mem_is_agg(const struct Mem *mem) { 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) { - 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)); (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)); 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)); 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)); 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)); 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); -- 2.25.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/3] sql: make mem_is_bin() to check only for VARBINARY 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 2021-05-17 12:05 ` Mergen Imeev via Tarantool-patches 0 siblings, 1 reply; 6+ messages in thread From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-29 21:05 UTC (permalink / raw) To: imeevma; +Cc: tarantool-patches 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); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/3] sql: make mem_is_bin() to check only for VARBINARY 2021-04-29 21:05 ` Vladislav Shpilevoy via Tarantool-patches @ 2021-05-17 12:05 ` Mergen Imeev via Tarantool-patches 0 siblings, 0 replies; 6+ messages in thread From: Mergen Imeev via Tarantool-patches @ 2021-05-17 12:05 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Thank you for the review! My answers, diff and new patch below. On Thu, Apr 29, 2021 at 11:05:19PM +0200, Vladislav Shpilevoy wrote: > 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() > Fixed. However, it turned out that you suggestion made diff even less, though you suggested to use two or three functions instead of one. > 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? > I changed here, however I do not think that it should be done this way, since we have to take into account our implicit cast rules and change this code every time we will add something, that can be converted to value of accepted type. Instead, all check and convertions should be done before the function is called. This will be done in issue #4159, which was already partially done last year. Last time I wasn't able to complete this issue, but we were able to come to a conclusion about how should it be done. I think I will return to the issue in Q3 or Q4. > > 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. > Fixed. > > 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. > Fixed. > > 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. > In current design TRIM() accepts any argument. I reverted this change, however it should not affect MAP and ARRAY values. > > 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. > Fixed. > > 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. > I added an error on attempt to compare MAP or ARRAY to anything. > > - 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? > It is true, that MEM contains MP_ARRAY. However, currently this MEM has VARBINARY type. I tried to change type to ARRAY, but found unexpected for me error: ARRAY cannot be inserted in SCALAR field, but VARBINARY can. Also, we cannot create an index that contains MAP or ARRAY.It means that after changing type to ARRAY, value created by OP_MakeRecord cannot be inserted to most of ephemeral spaces, since all their fields have VARBINARY type and their PK consist of all fields. I decided to drop my attempt and accept that currently all internally created arrays are of VARBINARY type. Also, this means that arrays and maps have undefined behaviour in our code, since their behaviour depends on the source of the value. So, for now most of maps and arrays we can just use mem_is_bin(), at least for now. I think that we have to rework out ephemeral spaces if we want to introduce support of MAP and ARRAY in SQL. > > (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. > Reverted. > > 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. > Reverted. > > 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. > Reverted. > > 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. > Reverted. > > 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); Diff: diff --git a/src/box/sql/func.c b/src/box/sql/func.c index ae5ead67e..90e8e152f 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_ext(argv[0])) { + if (!mem_is_num(argv[0]) && !mem_is_str(argv[0])) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(argv[0]), "numeric"); context->is_aborted = true; @@ -613,7 +613,8 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv) \ const char *z2; \ int n; \ UNUSED_PARAMETER(argc); \ - if (mem_is_bin_ext(argv[0])) { \ + if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) || \ + mem_is_array(argv[0])) { \ diag_set(ClientError, ER_INCONSISTENT_TYPES, "text", \ "varbinary"); \ context->is_aborted = true; \ @@ -694,7 +695,8 @@ randomBlob(sql_context * context, int argc, sql_value ** argv) unsigned char *p; assert(argc == 1); UNUSED_PARAMETER(argc); - if (mem_is_bin_ext(argv[0])) { + if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) || + mem_is_array(argv[0])) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(argv[0]), "numeric"); context->is_aborted = true; @@ -1455,7 +1457,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_ext(arg)) + if (mem_is_bin(arg)) default_trim = (const unsigned char *) "\0"; else default_trim = (const unsigned char *) " "; @@ -1584,7 +1586,8 @@ 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_ext(argv[0])) { + if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) || + mem_is_array(argv[0])) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(argv[0]), "text"); context->is_aborted = true; diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index 6fc15617d..526b6bf3e 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -206,12 +206,6 @@ mem_is_array(const struct Mem *mem) mp_typeof(*mem->z) == MP_ARRAY; } -static inline bool -mem_is_bin_ext(const struct Mem *mem) -{ - return (mem->flags & MEM_Blob) != 0; -} - static inline bool mem_is_agg(const struct Mem *mem) { diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index bedfa87af..12ec703a2 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1634,15 +1634,20 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ "boolean"); goto abort_due_to_error; } - } else if (mem_is_bin_ext(pIn3) || mem_is_bin_ext(pIn1)) { + } else if (mem_is_bin(pIn3) || mem_is_bin(pIn1)) { if (mem_cmp_bin(pIn3, pIn1, &res) != 0) { - char *str = !mem_is_bin_ext(pIn3) ? + char *str = !mem_is_bin(pIn3) ? mem_type_to_str(pIn3) : mem_type_to_str(pIn1); diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str, "varbinary"); goto abort_due_to_error; } + } else if (mem_is_map(pIn3) || mem_is_map(pIn1) || mem_is_array(pIn3) || + mem_is_array(pIn1)) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_type_to_str(pIn3), mem_type_to_str(pIn1)); + goto abort_due_to_error; } else if (type == FIELD_TYPE_STRING) { if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) { const char *str = @@ -2991,7 +2996,7 @@ case OP_Found: { /* jump, in3 */ } else { pFree = pIdxKey = sqlVdbeAllocUnpackedRecord(db, pC->key_def); if (pIdxKey==0) goto no_mem; - assert(mem_is_bin_ext(pIn3)); + assert(mem_is_bin(pIn3)); (void)ExpandBlob(pIn3); sqlVdbeRecordUnpackMsgpack(pC->key_def, pIn3->z, pIdxKey); @@ -3253,7 +3258,7 @@ case OP_SorterData: { assert(isSorter(pC)); if (sqlVdbeSorterRowkey(pC, pOut) != 0) goto abort_due_to_error; - assert(mem_is_bin_ext(pOut)); + assert(mem_is_bin(pOut)); assert(pOp->p1>=0 && pOp->p1<p->nCursor); p->apCsr[pOp->p3]->cacheStatus = CACHE_STALE; break; @@ -3616,7 +3621,7 @@ case OP_SorterInsert: { /* in2 */ assert(cursor != NULL); assert(isSorter(cursor)); pIn2 = &aMem[pOp->p2]; - assert(mem_is_bin_ext(pIn2)); + assert(mem_is_bin(pIn2)); if (ExpandBlob(pIn2) != 0 || sqlVdbeSorterWrite(cursor, pIn2) != 0) goto abort_due_to_error; @@ -3650,7 +3655,7 @@ case OP_SorterInsert: { /* in2 */ case OP_IdxReplace: case OP_IdxInsert: { pIn2 = &aMem[pOp->p1]; - assert(mem_is_bin_ext(pIn2)); + assert(mem_is_bin(pIn2)); if (ExpandBlob(pIn2) != 0) goto abort_due_to_error; struct space *space; @@ -3741,10 +3746,10 @@ case OP_Update: { assert(pOp->p4type == P4_SPACEPTR); struct Mem *key_mem = &aMem[pOp->p2]; - assert(mem_is_bin_ext(key_mem)); + assert(mem_is_bin(key_mem)); struct Mem *upd_fields_mem = &aMem[pOp->p3]; - assert(mem_is_bin_ext(upd_fields_mem)); + assert(mem_is_bin(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); Patch: commit f6ad3ac1c548a2ffbfbe4b103b533f48db6458fa Author: Mergen Imeev <imeevma@gmail.com> Date: Tue Apr 27 15:12:33 2021 +0300 sql: make mem_is_bin() to check only for VARBINARY 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 diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 94302cc56..90e8e152f 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_num(argv[0]) && !mem_is_str(argv[0])) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(argv[0]), "numeric"); context->is_aborted = true; @@ -613,7 +613,8 @@ 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(argv[0]) || mem_is_map(argv[0]) || \ + mem_is_array(argv[0])) { \ diag_set(ClientError, ER_INCONSISTENT_TYPES, "text", \ "varbinary"); \ context->is_aborted = true; \ @@ -694,7 +695,8 @@ 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(argv[0]) || mem_is_map(argv[0]) || + mem_is_array(argv[0])) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(argv[0]), "numeric"); context->is_aborted = true; @@ -1584,7 +1586,8 @@ 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(argv[0]) || mem_is_map(argv[0]) || + mem_is_array(argv[0])) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(argv[0]), "text"); context->is_aborted = true; diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index 1db7f4deb..526b6bf3e 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -184,7 +184,7 @@ mem_is_bool(const struct Mem *mem) static inline bool mem_is_bin(const struct Mem *mem) { - return (mem->flags & MEM_Blob) != 0; + return (mem->flags & MEM_Blob) != 0 && (mem->flags & MEM_Subtype) == 0; } static inline bool diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 2308587e7..12ec703a2 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1643,6 +1643,11 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ "varbinary"); goto abort_due_to_error; } + } else if (mem_is_map(pIn3) || mem_is_map(pIn1) || mem_is_array(pIn3) || + mem_is_array(pIn1)) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_type_to_str(pIn3), mem_type_to_str(pIn1)); + goto abort_due_to_error; } else if (type == FIELD_TYPE_STRING) { if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) { const char *str = ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-17 12:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[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
2021-05-17 12:05 ` Mergen Imeev via Tarantool-patches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox