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

[Vladislav Shpilevoy]

Thanks for the fixes!

On 29/07/2019 02:03, n.pettik wrote:
> 
>>
>> 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.

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.

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

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?