From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 3/9] sql: use msgpack types instead of custom ones Date: Thu, 18 Apr 2019 20:54:37 +0300 [thread overview] Message-ID: <BD1C58E8-5F3B-4874-A35E-B2A71E006460@tarantool.org> (raw) In-Reply-To: <374916ec-2e49-643f-eccb-42f9d66e340e@tarantool.org> >> diff --git a/src/box/bind.c b/src/box/bind.c >> index 4b57e23c8..5aa79751a 100644 >> --- a/src/box/bind.c >> +++ b/src/box/bind.c >> @@ -138,6 +121,7 @@ sql_bind_decode(struct sql_bind *bind, int i, const char **packet) >> default: >> unreachable(); >> } >> + bind->type = type; > > 1. struct sql_bind.type still is uint8_t and commented as > 'SQL type of the value.'. Please, change its type to enum mp_type > and say in the comment 'MessagePack type of the value.’ diff --git a/src/box/bind.h b/src/box/bind.h index cacb4a2d9..a915381e4 100644 --- a/src/box/bind.h +++ b/src/box/bind.h @@ -39,6 +39,8 @@ extern "C" { #include <stdint.h> #include <stdlib.h> +#include "msgpuck.h" + struct sql_stmt; /** @@ -55,8 +57,8 @@ struct sql_bind { /** Byte length of the value. */ uint32_t bytes; - /** SQL type of the value. */ - uint8_t type; + /** MessagePack type of the value. */ + enum mp_type type; /** Bind value. */ union { double d; >>> diff --git a/src/box/sql/date.c b/src/box/sql/date.c >> index 5f5272ea3..ead0c01d0 100644 >> --- a/src/box/sql/date.c >> +++ b/src/box/sql/date.c >> @@ -937,7 +937,7 @@ isDate(sql_context * context, int argc, sql_value ** argv, DateTime * p) >> if (argc == 0) { >> return setDateTimeToCurrent(context, p); >> } >> - if ((eType = sql_value_type(argv[0])) == SQL_FLOAT >> + if ((eType = sql_value_type(argv[0])) == MP_DOUBLE > > 2. Firstly, the code is commented out. Secondly, sql_value_type > does not exist anymore. Thirdly, eType is of type int, not > enum mp_type, and can not be compared with MP_DOUBLE. > > Please, rename sql_value_mp_type to sql_value_type, because we > do not have 'not mp' type anymore. Then this hunk will be ok > except the third objection. Ok, renamed and fixed type of eType (nevertheless that chunk is related to dead code). diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 3cdb119c8..667837528 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -87,10 +87,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv) pColl = sqlGetFuncCollSeq(context); assert(mask == -1 || mask == 0); iBest = 0; - if (sql_value_mp_type(argv[0]) == MP_NIL) + if (sql_value_type(argv[0]) == MP_NIL) return; for (i = 1; i < argc; i++) { - if (sql_value_mp_type(argv[i]) == MP_NIL) + if (sql_value_type(argv[i]) == MP_NIL) return; if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >= 0) { @@ -109,7 +109,7 @@ typeofFunc(sql_context * context, int NotUsed, sql_value ** argv) { const char *z = 0; UNUSED_PARAMETER(NotUsed); - switch (sql_value_mp_type(argv[0])) { + switch (sql_value_type(argv[0])) { case MP_INT: z = "integer"; break; @@ -139,7 +139,7 @@ lengthFunc(sql_context * context, int argc, sql_value ** argv) assert(argc == 1); UNUSED_PARAMETER(argc); - switch (sql_value_mp_type(argv[0])) { + switch (sql_value_type(argv[0])) { case MP_BIN: case MP_INT: case MP_DOUBLE:{ @@ -173,7 +173,7 @@ absFunc(sql_context * context, int argc, sql_value ** argv) { assert(argc == 1); UNUSED_PARAMETER(argc); - switch (sql_value_mp_type(argv[0])) { + switch (sql_value_type(argv[0])) { case MP_INT:{ i64 iVal = sql_value_int64(argv[0]); if (iVal < 0) { @@ -229,8 +229,8 @@ position_func(struct sql_context *context, int argc, struct Mem **argv) UNUSED_PARAMETER(argc); struct Mem *needle = argv[0]; struct Mem *haystack = argv[1]; - enum mp_type needle_type = sql_value_mp_type(needle); - enum mp_type haystack_type = sql_value_mp_type(haystack); + enum mp_type needle_type = sql_value_type(needle); + enum mp_type haystack_type = sql_value_type(haystack); if (haystack_type == MP_NIL || needle_type == MP_NIL) return; @@ -398,12 +398,12 @@ substrFunc(sql_context * context, int argc, sql_value ** argv) int negP2 = 0; assert(argc == 3 || argc == 2); - if (sql_value_mp_type(argv[1]) == MP_NIL - || (argc == 3 && sql_value_mp_type(argv[2]) == MP_NIL) + if (sql_value_type(argv[1]) == MP_NIL + || (argc == 3 && sql_value_type(argv[2]) == MP_NIL) ) { return; } - p0type = sql_value_mp_type(argv[0]); + p0type = sql_value_type(argv[0]); p1 = sql_value_int(argv[1]); if (p0type == MP_BIN) { len = sql_value_bytes(argv[0]); @@ -507,13 +507,13 @@ roundFunc(sql_context * context, int argc, sql_value ** argv) char *zBuf; assert(argc == 1 || argc == 2); if (argc == 2) { - if (MP_NIL == sql_value_mp_type(argv[1])) + if (MP_NIL == sql_value_type(argv[1])) return; n = sql_value_int(argv[1]); if (n < 0) n = 0; } - if (sql_value_mp_type(argv[0]) == MP_NIL) + if (sql_value_type(argv[0]) == MP_NIL) return; r = sql_value_double(argv[0]); /* If Y==0 and X will fit in a 64-bit int, @@ -900,8 +900,8 @@ likeFunc(sql_context *context, int argc, sql_value **argv) int nPat; sql *db = sql_context_db_handle(context); int is_like_ci = SQL_PTR_TO_INT(sql_user_data(context)); - int rhs_type = sql_value_mp_type(argv[0]); - int lhs_type = sql_value_mp_type(argv[1]); + int rhs_type = sql_value_type(argv[0]); + int lhs_type = sql_value_type(argv[1]); if (lhs_type != MP_STR || rhs_type != MP_STR) { if (lhs_type == MP_NIL || rhs_type == MP_NIL) @@ -1018,7 +1018,7 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv) { assert(argc == 1); UNUSED_PARAMETER(argc); - switch (sql_value_mp_type(argv[0])) { + switch (sql_value_type(argv[0])) { case MP_DOUBLE:{ double r1, r2; char zBuf[50]; @@ -1092,7 +1092,7 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv) break; } default:{ - assert(sql_value_mp_type(argv[0]) == MP_NIL); + assert(sql_value_type(argv[0]) == MP_NIL); sql_result_text(context, "NULL", 4, SQL_STATIC); break; } @@ -1228,13 +1228,13 @@ replaceFunc(sql_context * context, int argc, sql_value ** argv) assert(zStr == sql_value_text(argv[0])); /* No encoding change */ zPattern = sql_value_text(argv[1]); if (zPattern == 0) { - assert(sql_value_mp_type(argv[1]) == MP_NIL + assert(sql_value_type(argv[1]) == MP_NIL || sql_context_db_handle(context)->mallocFailed); return; } nPattern = sql_value_bytes(argv[1]); if (nPattern == 0) { - assert(sql_value_mp_type(argv[1]) != MP_NIL); + assert(sql_value_type(argv[1]) != MP_NIL); sql_result_value(context, argv[0]); return; } @@ -1302,7 +1302,7 @@ trimFunc(sql_context * context, int argc, sql_value ** argv) unsigned char **azChar = 0; /* Individual characters in zCharSet */ int nChar; /* Number of characters in zCharSet */ - if (sql_value_mp_type(argv[0]) == MP_NIL) { + if (sql_value_type(argv[0]) == MP_NIL) { return; } zIn = sql_value_text(argv[0]); @@ -1499,7 +1499,7 @@ sumStep(sql_context * context, int argc, sql_value ** argv) UNUSED_PARAMETER(argc); p = sql_aggregate_context(context, sizeof(*p)); assert(p != NULL); - enum mp_type type = sql_value_mp_type(argv[0]); + enum mp_type type = sql_value_type(argv[0]); if (type == MP_NIL) return; if (type != MP_DOUBLE && type != MP_INT) { @@ -1510,7 +1510,7 @@ sumStep(sql_context * context, int argc, sql_value ** argv) context->isError = SQL_TARANTOOL_ERROR; return; } - type = sql_value_mp_type(argv[0]); + type = sql_value_type(argv[0]); } p->cnt++; if (type == MP_INT) { @@ -1578,7 +1578,7 @@ countStep(sql_context * context, int argc, sql_value ** argv) { CountCtx *p; p = sql_aggregate_context(context, sizeof(*p)); - if ((argc == 0 || MP_NIL != sql_value_mp_type(argv[0])) && p) { + if ((argc == 0 || MP_NIL != sql_value_type(argv[0])) && p) { p->n++; } } @@ -1605,7 +1605,7 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) if (!pBest) return; - if (sql_value_mp_type(argv[0]) == MP_NIL) { + if (sql_value_type(argv[0]) == MP_NIL) { if (pBest->flags) sqlSkipAccumulatorLoad(context); } else if (pBest->flags) { @@ -1657,7 +1657,7 @@ groupConcatStep(sql_context * context, int argc, sql_value ** argv) const char *zSep; int nVal, nSep; assert(argc == 1 || argc == 2); - if (sql_value_mp_type(argv[0]) == MP_NIL) + if (sql_value_type(argv[0]) == MP_NIL) return; pAccum = (StrAccum *) sql_aggregate_context(context, sizeof(*pAccum)); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 9134f767d..3b15d9775 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -456,7 +456,7 @@ const unsigned char * sql_value_text(sql_value *); enum mp_type -sql_value_mp_type(sql_value *); +sql_value_type(sql_value *); sql * sql_context_db_handle(sql_context *); diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 6e867ca84..149f3d047 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -244,7 +244,7 @@ sql_value_text(sql_value * pVal) * point number string BLOB NULL */ enum mp_type -sql_value_mp_type(sql_value *pVal) +sql_value_type(sql_value *pVal) { switch (pVal->flags & MEM_PURE_TYPE_MASK) { case MEM_Int: return MP_INT; @@ -1013,7 +1013,7 @@ sql_column_value(sql_stmt * pStmt, int i) enum mp_type sql_column_type(sql_stmt * pStmt, int i) { - enum mp_type type = sql_value_mp_type(columnMem(pStmt, i)); + enum mp_type type = sql_value_type(columnMem(pStmt, i)); columnMallocFailure(pStmt); return type; } @@ -1388,7 +1388,7 @@ int sql_bind_value(sql_stmt * pStmt, int i, const sql_value * pValue) { int rc; - switch (sql_value_mp_type((sql_value *) pValue)) { + switch (sql_value_type((sql_value *) pValue)) { case MP_INT:{ rc = sql_bind_int64(pStmt, i, pValue->u.i); break; diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c index a80c7a0ab..30017b080 100644 --- a/src/box/sql/whereexpr.c +++ b/src/box/sql/whereexpr.c @@ -290,7 +290,7 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix, pVal = sqlVdbeGetBoundValue(pReprepare, iCol, FIELD_TYPE_SCALAR); - if (pVal && sql_value_mp_type(pVal) == MP_STR) { + if (pVal && sql_value_type(pVal) == MP_STR) { z = (char *)sql_value_text(pVal); } >> || eType == SQL_INTEGER) { >> setRawDateNumber(p, sql_value_double(argv[0])); >> } else { >> diff --git a/src/box/sql/func.c b/src/box/sql/func.c >> index 9adfeec67..3cdb119c8 100644 >> --- a/src/box/sql/func.c >> +++ b/src/box/sql/func.c >> @@ -87,10 +87,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv) >> pColl = sqlGetFuncCollSeq(context); >> assert(mask == -1 || mask == 0); >> iBest = 0; >> - if (sql_value_type(argv[0]) == SQL_NULL) >> + if (sql_value_mp_type(argv[0]) == MP_NIL) > > 3. Please, move that check into a separate static inline > function in a header file. It appeared to be used often. > A possible name 'sql_value_is_null’. diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 3cdb119c8..66f601346 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -87,10 +87,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv) pColl = sqlGetFuncCollSeq(context); assert(mask == -1 || mask == 0); iBest = 0; - if (sql_value_mp_type(argv[0]) == MP_NIL) + if (sql_value_is_null(argv[0])) return; for (i = 1; i < argc; i++) { - if (sql_value_mp_type(argv[i]) == MP_NIL) + if (sql_value_is_null(argv[i])) return; if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >= 0) { @@ -398,12 +398,12 @@ substrFunc(sql_context * context, int argc, sql_value ** argv) int negP2 = 0; assert(argc == 3 || argc == 2); - if (sql_value_mp_type(argv[1]) == MP_NIL - || (argc == 3 && sql_value_mp_type(argv[2]) == MP_NIL) + if (sql_value_is_null(argv[1]) + || (argc == 3 && sql_value_is_null(argv[2])) ) { return; } @@ -507,13 +507,13 @@ roundFunc(sql_context * context, int argc, sql_value ** argv) char *zBuf; assert(argc == 1 || argc == 2); if (argc == 2) { - if (MP_NIL == sql_value_mp_type(argv[1])) + if (sql_value_is_null(argv[1])) return; n = sql_value_int(argv[1]); if (n < 0) n = 0; } - if (sql_value_mp_type(argv[0]) == MP_NIL) + if (sql_value_is_null(argv[0])) return; r = sql_value_double(argv[0]); /* If Y==0 and X will fit in a 64-bit int, @@ -1092,7 +1092,7 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv) break; } default:{ - assert(sql_value_mp_type(argv[0]) == MP_NIL); + assert(sql_value_is_null(argv[0])); sql_result_text(context, "NULL", 4, SQL_STATIC); break; } @@ -1228,13 +1228,13 @@ replaceFunc(sql_context * context, int argc, sql_value ** argv) assert(zStr == sql_value_text(argv[0])); /* No encoding change */ zPattern = sql_value_text(argv[1]); if (zPattern == 0) { - assert(sql_value_mp_type(argv[1]) == MP_NIL + assert(sql_value_is_null(argv[1]) || sql_context_db_handle(context)->mallocFailed); return; } nPattern = sql_value_bytes(argv[1]); if (nPattern == 0) { - assert(sql_value_mp_type(argv[1]) != MP_NIL); + assert(! sql_value_is_null(argv[1])); sql_result_value(context, argv[0]); return; } @@ -1302,7 +1302,7 @@ trimFunc(sql_context * context, int argc, sql_value ** argv) unsigned char **azChar = 0; /* Individual characters in zCharSet */ int nChar; /* Number of characters in zCharSet */ - if (sql_value_mp_type(argv[0]) == MP_NIL) { + if (sql_value_is_null(argv[0])) { return; } zIn = sql_value_text(argv[0]); @@ -1578,7 +1578,7 @@ countStep(sql_context * context, int argc, sql_value ** argv) { CountCtx *p; p = sql_aggregate_context(context, sizeof(*p)); - if ((argc == 0 || MP_NIL != sql_value_mp_type(argv[0])) && p) { + if ((argc == 0 || ! sql_value_is_null(argv[0])) && p) { p->n++; } } @@ -1605,7 +1605,7 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) if (!pBest) return; - if (sql_value_mp_type(argv[0]) == MP_NIL) { + if (sql_value_is_null(argv[0])) { if (pBest->flags) sqlSkipAccumulatorLoad(context); } else if (pBest->flags) { @@ -1657,7 +1657,7 @@ groupConcatStep(sql_context * context, int argc, sql_value ** argv) const char *zSep; int nVal, nSep; assert(argc == 1 || argc == 2); - if (sql_value_mp_type(argv[0]) == MP_NIL) + if (sql_value_is_null(argv[0])) return; pAccum = (StrAccum *) sql_aggregate_context(context, sizeof(*pAccum)); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 9134f767d..2189d8c3e 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -456,7 +456,13 @@ const unsigned char * sql_value_text(sql_value *); +static inline bool +sql_value_is_null(sql_value *value) +{ + return sql_value_type(value) == MP_NIL; +} >> return; >> for (i = 1; i < argc; i++) { >> - if (sql_value_type(argv[i]) == SQL_NULL) >> + if (sql_value_mp_type(argv[i]) == MP_NIL) >> return; >> if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >= >> 0) { >> @@ -139,15 +139,15 @@ lengthFunc(sql_context * context, int argc, sql_value ** argv) >> >> assert(argc == 1); >> UNUSED_PARAMETER(argc); >> - switch (sql_value_type(argv[0])) { >> - case SQL_BLOB: >> - case SQL_INTEGER: >> - case SQL_FLOAT:{ >> + switch (sql_value_mp_type(argv[0])) { >> + case MP_BIN: >> + case MP_INT: >> + case MP_DOUBLE:{ > > 4. Probably you could fix part of this: > https://github.com/tarantool/tarantool/issues/4159 in scope of > this commit, alongside. THis patch provides straightforward refactoring, I don’t want to involve here non-trivial changes.
next prev parent reply other threads:[~2019-04-18 17:54 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-14 15:03 [tarantool-patches] [PATCH 0/9] Introduce type BOOLEAN in SQL Nikita Pettik 2019-04-14 15:03 ` [tarantool-patches] [PATCH 1/9] sql: refactor mem_apply_numeric_type() Nikita Pettik 2019-04-14 15:04 ` [tarantool-patches] [PATCH 2/9] sql: disallow text values participate in sum() aggregate Nikita Pettik 2019-04-16 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2019-04-18 17:54 ` n.pettik 2019-04-22 18:02 ` Vladislav Shpilevoy 2019-04-23 19:58 ` n.pettik 2019-04-14 15:04 ` [tarantool-patches] [PATCH 3/9] sql: use msgpack types instead of custom ones Nikita Pettik 2019-04-16 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2019-04-18 17:54 ` n.pettik [this message] 2019-04-22 18:02 ` Vladislav Shpilevoy 2019-04-23 19:58 ` n.pettik 2019-04-14 15:04 ` [tarantool-patches] [PATCH 4/9] sql: introduce type boolean Nikita Pettik 2019-04-16 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2019-04-18 17:54 ` n.pettik 2019-04-22 18:02 ` Vladislav Shpilevoy 2019-04-23 19:58 ` n.pettik 2019-04-23 21:06 ` Vladislav Shpilevoy 2019-04-14 15:04 ` [tarantool-patches] [PATCH 5/9] sql: improve type determination for column meta Nikita Pettik 2019-04-16 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2019-04-18 17:54 ` n.pettik 2019-04-22 18:02 ` Vladislav Shpilevoy 2019-04-23 19:58 ` n.pettik 2019-04-14 15:04 ` [tarantool-patches] [PATCH 6/9] sql: make comparison predicate return boolean Nikita Pettik 2019-04-16 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2019-04-18 17:54 ` n.pettik 2019-04-14 15:04 ` [tarantool-patches] [PATCH 7/9] sql: make predicates accept and " Nikita Pettik 2019-04-16 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2019-04-18 17:55 ` n.pettik 2019-04-14 15:04 ` [tarantool-patches] [PATCH 9/9] sql: make <search condition> accept only boolean Nikita Pettik 2019-04-16 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2019-04-18 17:55 ` n.pettik 2019-04-22 18:02 ` Vladislav Shpilevoy 2019-04-23 19:59 ` n.pettik 2019-04-23 21:06 ` Vladislav Shpilevoy 2019-04-23 22:01 ` n.pettik [not found] ` <b2a84f129c2343d3da3311469cbb7b20488a21c2.1555252410.git.korablev@tarantool.org> 2019-04-16 14:12 ` [tarantool-patches] Re: [PATCH 8/9] sql: make LIKE predicate return boolean result Vladislav Shpilevoy 2019-04-18 17:55 ` n.pettik 2019-04-22 18:02 ` Vladislav Shpilevoy 2019-04-23 19:58 ` n.pettik 2019-04-24 10:28 ` [tarantool-patches] Re: [PATCH 0/9] Introduce type BOOLEAN in SQL Vladislav Shpilevoy 2019-04-25 8:46 ` Kirill Yukhin
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=BD1C58E8-5F3B-4874-A35E-B2A71E006460@tarantool.org \ --to=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH 3/9] sql: use msgpack types instead of custom ones' \ /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