[Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values

[Vladislav Shpilevoy]

Thanks for the fixes!

>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>>> index 55e494332..86de3f98a 100644
>>> --- a/src/box/sql/vdbe.c
>>> +++ b/src/box/sql/vdbe.c
>>> @@ -1438,6 +1438,26 @@ case OP_Array: {
>>>  	break;
>>>  }
>>>  
>>> +/**
>>> + * Opcode: Map P1 P2 P3 * *
>>> + * Synopsis: r[P2] = map(P3 at P1)
>>> + *
>>> + * Construct an MAP value from P1 registers starting at reg(P3).
>>> + */
>>> +case OP_Map: {
>>> +	pOut = &aMem[pOp->p2];
>>> +
>>> +	uint32_t size;
>>> +	struct region *region = &fiber()->gc;
>>> +	size_t svp = region_used(region);
>>> +	char *val = mem_encode_map(&aMem[pOp->p3], pOp->p1, &size, region);
>>> +	if (val == NULL || mem_copy_map(pOut, val, size) != 0) {
>>> +		region_truncate(region, svp);
>>> +		goto abort_due_to_error;
>>> +	}
>>
>> You should probably truncate the region regardless of the result. Otherwise
>> in case of success you will leak the region inside of the query bit by bit
>> while SELECT works:
>>
>> 	box.execute('CREATE TABLE test (id INTEGER PRIMARY KEY)')
>> 	box.execute('INSERT INTO test VALUES (1), (2), (3)')
>> 	box.execute('SELECT {id: id} FROM test')
>>
>> Here you will do OP_Map 3 times, all will leave some region memory leaked
>> every time. It is freed in the end of execution probably, but it might do
>> some big usage while the request is in progress when the row count is much
>> bigger than 3.
>>
> Thank you! Fixed.
> 
>> Btw, worth adding a multirow test. All current map tests select a single row.
> There are several tests where the number of selected rows is more than one, for
> example map-3 or map-4, but the number of rows is still quite low. I added
> another test that selects 1000 rows with maps.

I meant this case when you select not just already stored values, but
construct a map from the select values. For instance, not just

	SELECT col FROM t;

where 'col' is map, but rather

	SELECT {key: value} FROM t;

where 'key' and 'value' are columns in T and row count > 0.
In your new test you select '*', but I wanted to test what
happens when columns are used inside of map syntax + row count > 0.

Anyway it works, so the patchset LGTM.