From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 5/5] sql: introduce VARBINARY column type Date: Mon, 29 Jul 2019 03:03:29 +0300 [thread overview] Message-ID: <1293710C-85CC-4F5A-A7CF-4677D901DCE9@tarantool.org> (raw) In-Reply-To: <49a188eb-dafe-44e7-a0fd-e9244b68e721@tarantool.org> > > 1. Nit: > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index d0f0cb4f5..ea7c9f25f 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2098,8 +2098,7 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ > } > break; > } > - } else if (((flags1 | flags3) & MEM_Bool) != 0 || > - ((flags1 | flags3) & MEM_Blob) != 0) { > + } else if (((flags1 | flags3) & (MEM_Bool | MEM_Blob)) != 0) { > /* > * If one of values is of type BOOLEAN or VARBINARY, > * then the second one must be of the same type as Ok, applied. > 2. Please, add tests on varbinary values binding. Could you suggest the way to force MP_BIN format from Lua? I see the only test on VARBINARY type (test/box/varbinary_type.test.lua), but this approach is unlikely to be applicable for bindings. > 3. BLOB keyword is reserved, but also it is used in parse.y:980. > Should not it be deleted from all the rules, and be just > reserved? It’s rule for declaring blob (aka binary string) literals. I can rename it, but TBO it looks OK to me. > 4. Additionally (I bet, you knew I would ask), what are we going > to do with all mentionings of blob in the code? We have word 'blob' > mentioned 368 times in all the SQL sources, including comments, > parts of names. I’m OK with using BLOB as a synonym to entity of type varbinary. Can manually clean-out all these places, but a bit later (after 2.2 release). > In wherecode.c:644 we have a CREATE TABLE example, using BLOB - > this one definitely should be fixed. What about other places? > We could replace 'blob' -> 'bin','binary', 'vbin', ... . It > should not take much time. This path has been removed on master branch. So I simply rebased current branch and it disappeared. I’ve also extended patch-set setting correct default trim octet for binary string arguments: sql: fix default trim octet for binary strings According to ANSI specification, if TRIM function accepts binary string and trim octet is not specified, then it is implicitly set to X'00'. Before this patch trim octet was set to ' ' both for string and binary string arguments. In turn, ' ' is equal to X'20' in hex representation. Hence, TRIM function cut wrong characters: TRIM(X'004420') -> X‘0044' This patch sets default trim octet to X'00' for binary string arguments. Part of #4206 diff --git a/src/box/sql/func.c b/src/box/sql/func.c index e799f380f..9bea3eda0 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -1397,15 +1397,20 @@ trim_func_one_arg(struct sql_context *context, int argc, sql_value **argv) { assert(argc == 1); (void) argc; - - const unsigned char *input_str; - if ((input_str = sql_value_text(argv[0])) == NULL) + /* In case of VARBINARY type default trim octet is X'00'. */ + const unsigned char *default_trim; + enum mp_type val_type = sql_value_type(argv[0]); + if (val_type == MP_NIL) return; - + if (val_type == MP_BIN) + default_trim = (const unsigned char *) "\0"; + else + default_trim = (const unsigned char *) " "; int input_str_sz = sql_value_bytes(argv[0]); - uint8_t len_one = 1; - trim_procedure(context, TRIM_BOTH, (const unsigned char *) " ", - &len_one, 1, input_str, input_str_sz); + const unsigned char *input_str = sql_value_text(argv[0]); + uint8_t trim_char_len[1] = { 1 }; + trim_procedure(context, TRIM_BOTH, default_trim, trim_char_len, 1, + input_str, input_str_sz); } /** diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua index e073937b8..b81520e33 100755 --- a/test/sql-tap/func.test.lua +++ b/test/sql-tap/func.test.lua @@ -2232,6 +2232,24 @@ test:do_catchsql_test( -- </func-22.38> }) +test:do_execsql_test( + "func-22.39", + [[ + SELECT HEX(TRIM(X'004420')) + ]], { "4420" }) + +test:do_execsql_test( + "func-22.40", + [[ + SELECT HEX(TRIM(X'00442000')) + ]], { "4420" }) + +test:do_execsql_test( + "func-22.41", + [[ + SELECT HEX(TRIM(X'442000')) + ]], { "4420" }) + -- This is to test the deprecated sql_aggregate_count() API. -- --test:do_test(
next prev parent reply other threads:[~2019-07-29 0:03 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-24 11:42 [tarantool-patches] [PATCH 0/5] Introduce VARBINARY in SQL Nikita Pettik 2019-07-24 11:42 ` [tarantool-patches] [PATCH 1/5] sql: always erase numeric flag after stringifying Nikita Pettik 2019-07-24 11:42 ` [tarantool-patches] [PATCH 2/5] sql: fix resulting type calculation for CASE-WHEN stmt Nikita Pettik 2019-07-25 22:12 ` [tarantool-patches] " Vladislav Shpilevoy [not found] ` <a061e845-eeb1-00d1-9141-3b9bb87768f5@tarantool.org> 2019-07-28 23:56 ` n.pettik 2019-07-24 11:42 ` [tarantool-patches] [PATCH 3/5] sql: use 'varbinary' as a name of type instead of 'blob' Nikita Pettik 2019-07-25 22:11 ` [tarantool-patches] " Vladislav Shpilevoy [not found] ` <2e655514-0fec-8baf-20a8-d49e5586b047@tarantool.org> 2019-07-28 23:56 ` n.pettik 2019-07-29 21:03 ` Vladislav Shpilevoy 2019-07-30 13:43 ` n.pettik 2019-07-24 11:42 ` [tarantool-patches] [PATCH 4/5] sql: make built-ins raise errors for varbin args Nikita Pettik 2019-07-25 22:11 ` [tarantool-patches] " Vladislav Shpilevoy [not found] ` <05d15035-2552-1f05-b7ce-facfbbc3a520@tarantool.org> 2019-07-28 23:59 ` n.pettik 2019-07-24 11:42 ` [tarantool-patches] [PATCH 5/5] sql: introduce VARBINARY column type Nikita Pettik 2019-07-25 22:12 ` [tarantool-patches] " Vladislav Shpilevoy [not found] ` <49a188eb-dafe-44e7-a0fd-e9244b68e721@tarantool.org> 2019-07-29 0:03 ` n.pettik [this message] 2019-07-29 20:55 ` Vladislav Shpilevoy 2019-07-30 13:44 ` n.pettik 2019-07-30 19:41 ` Vladislav Shpilevoy 2019-07-30 19:52 ` Vladislav Shpilevoy 2019-07-31 14:51 ` n.pettik 2019-08-01 8:42 ` [tarantool-patches] Re: [PATCH 0/5] Introduce VARBINARY in SQL 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=1293710C-85CC-4F5A-A7CF-4677D901DCE9@tarantool.org \ --to=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH 5/5] sql: introduce VARBINARY column type' \ /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