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

[n.pettik]

>>> 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.
> 
> I can, but you won't like it.
> 
> 	box.cfg{listen = 3313}
> 	netbox = require('net.box')
> 	buffer = require('buffer')
> 	ffi = require('ffi')
> 
> 	ffi.cdef[[
> 	typedef struct box_tuple_format_t box_tuple_format_t;
> 	typedef struct box_tuple_t box_tuple_t;
> 
> 	int
> 	box_insert(uint32_t space_id, const char *tuple, const char *tuple_end,
> 		   box_tuple_t **result);
> 	]]
> 
> 	s = box.schema.create_space('test')
> 	pk = s:create_index('pk', {parts = {{1, 'scalar'}}})
> 
> 	box.schema.user.grant('guest','read, write, execute', 'universe')
> 	box.schema.user.grant('guest', 'create', 'space')
> 	cn = netbox.connect(box.cfg.listen)
> 
> 	data = buffer.static_alloc('char', 1 + 6)
> 	data[0] = 0x91
> 	data[1] = 0xc4
> 	data[2] = 3
> 	data[3] = string.byte('b')
> 	data[4] = string.byte('i')
> 	data[5] = string.byte('n')
> 	ffi.C.box_insert(s.id, data, data + 6, nil)
> 
> 	cn:execute('SELECT typeof(?);', s:select()[1])
> 
> This returns:
> 
> 	---
> 	- metadata:
> 	  - name: typeof(?)
> 	    type: string
> 	  rows:
> 	  - ['varbinary']
> 	...
> 
> I used the fact that netbox encodes 'execute' bind array as a
> tuple. So I inserted MP_BIN into a tuple, and used it as a
> bind array.

Ok, I can’t come up with better option, so I slightly
simplified your example: there’s no need to use FFI
to insert MP_BIN since we can use SQL facilities to do so.

diff --git a/test/sql/bind.test.lua b/test/sql/bind.test.lua
index 3e0eed29a..51a87b0ee 100644
--- a/test/sql/bind.test.lua
+++ b/test/sql/bind.test.lua
@@ -93,7 +93,20 @@ execute('SELECT :value', parameters)
 --
 execute('SELECT ? ', {18446744073709551615ULL})
 
+-- Make sure that VARBINARY values can be bound. Note that
+-- at the moment there's no direct way to encode value as MP_BIN,
+-- so we have to use workaround only with remote option.
+--
 test_run:cmd("setopt delimiter ';'")
+
+if remote then
+       execute("CREATE TABLE t(a VARBINARY PRIMARY KEY);")
+       execute("INSERT INTO t VALUES (X'00');")
+       res = execute("SELECT typeof(?);", box.space.T:select()[1])
+       assert(res['rows'][1][1] == "varbinary")
+       execute("DROP TABLE t;")
+end;
+
 if remote then
        cn:close()
        box.schema.user.revoke('guest', 'read, write, execute', 'universe')

> 
>>> 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.
> 
> Could you please provide an example, how to use BLOB keyword to
> declare a literal? I can't find any test, using BLOB in a query
> string for anything.

I meant that it’s not the rule that allows BLOB keyword but
the rule to process X’…’ literals. When string is processed by
lexer (tokenize.c) its token type is assigned to TK_BLOB.
BLOB keyword itself is banned in mkkeywordhash.c since
it has token type TK_STANDARD.
In case you suggest to rename token type from TK_BLOB
to smth else - I see no reason to do so, it’s quite brief and
clear. If some day we decide to add BLOB keyword, its type
will be TK_BLOB_KW to avoid any confusions.

> Also, I've found, that 'blob' type is still mentioned in
> sql-tap/e_expr.test.lua quite a lot. And in sql-tap/types2.test.lua,
> where it is still used as a column type. These tests are disabled.
> Why?

See https://github.com/tarantool/tarantool/issues/3102#issuecomment-437274405

Seems that they contain a lot of type mess, I guess.
Nothing prevents us from resurrecting them, expect for time.