[tarantool-patches] Re: [PATCH 5/5] sql: introduce VARBINARY column type

[n.pettik]

> 
> 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(