Tarantool development patches archive
 help / color / mirror / Atom feed
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(

  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