[tarantool-patches] Re: [PATCH] Set format for spaces with sysview engine

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

>> 1. Please, prepend a subsystem name to the commit title.
>> Here I would use 'sysview' (by analogue with 'memtx', 'vinyl').
>> Do not forget to omit capital letter in 'Set' when the prefix
>> is added.
> 
> Didn't know we have spacial prefix for sysview. Done.

As I know, we do not have a strict list of subsystems at all.
Or it is hidden somewhere in SOP, which would not make any sense,
because new ones appear too often.

See 3 comments below.

>>> diff --git a/src/box/index_def.h b/src/box/index_def.h
>>> index 7717ecd..628342d 100644
>>> --- a/src/box/index_def.h
>>> +++ b/src/box/index_def.h
>>> @@ -336,6 +336,17 @@ index_def_new(uint32_t space_id, uint32_t iid, const char *name,
>>>  	      const struct index_opts *opts,
>>>  	      struct key_def *key_def, struct key_def *pk_def);
>>>  
>>> +/**
>>> + * Create an array of key_defs from array of index definitions.
>> 9. It is not an array of index definitions. It is a list. The
>> same below about @param index_defs.
> 
> Done.

1. No, it isn't. You still say "from array of index definitions",
but it is not array. Please, fix.

> 
>> 10. Please, do not write comments just for the sake of them.
>> Here 'size' is an [out] variable. From your comment it looks
>> like it is a ready-at-hand size of index_defs list.
> 
> Fixed.

2. [out] should be specified after @param, not after parameter
name (your current version is @param size[out]). Source:
http://www.doxygen.nl/manual/commands.html#cmdparam

>>> diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result
>>> index ae04266..6b51566 100644
>>> --- a/test/box/access_sysview.result
>>> +++ b/test/box/access_sysview.result
>>> @@ -642,7 +643,72 @@ box.space._vspace.index[1]:select('xxx')
>>>  ...
>>>  box.space._vspace.index[1]:select(1)
>>>  ---
>>> -- error: 'Supplied key type of part 0 does not match index part type: expected string'
>>> +- - [257, 1, '_vinyl_deferred_delete', 'blackhole', 0, {'group_id': 1}, [{'name': 'space_id',
>>> +        'type': 'unsigned'}, {'name': 'lsn', 'type': 'unsigned'}, {'name': 'tuple',
>>> +        'type': 'array'}]]
>>> +  - [272, 1, '_schema', 'memtx', 0, {}, [{'type': 'string', 'name': 'key'}, {'type': 'any',
>>> +        'name': 'value', 'is_nullable': true}]]
>>> +  - [276, 1, '_collation', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {
>>> +        'name': 'name', 'type': 'string'}, {'name': 'owner', 'type': 'unsigned'},
>>> +      {'name': 'type', 'type': 'string'}, {'name': 'locale', 'type': 'string'}, {
>>> +        'name': 'opts', 'type': 'map'}]]
>>> +  - [280, 1, '_space', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
>>> +        'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'engine',
>>> +        'type': 'string'}, {'name': 'field_count', 'type': 'unsigned'}, {'name': 'flags',
>>> +        'type': 'map'}, {'name': 'format', 'type': 'array'}]]
>>> +  - [281, 1, '_vspace', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
>>> +        'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'engine',
>>> +        'type': 'string'}, {'name': 'field_count', 'type': 'unsigned'}, {'name': 'flags',
>>> +        'type': 'map'}, {'name': 'format', 'type': 'array'}]]
>>> +  - [284, 1, '_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
>>> +        'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'step',
>>> +        'type': 'integer'}, {'name': 'min', 'type': 'integer'}, {'name': 'max', 'type': 'integer'},
>>> +      {'name': 'start', 'type': 'integer'}, {'name': 'cache', 'type': 'integer'},
>>> +      {'name': 'cycle', 'type': 'boolean'}]]
>>> +  - [285, 1, '_sequence_data', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'},
>>> +      {'name': 'value', 'type': 'integer'}]]
>>> +  - [286, 1, '_vsequence', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'},
>>> +      {'name': 'owner', 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {
>>> +        'name': 'step', 'type': 'integer'}, {'name': 'min', 'type': 'integer'}, {
>>> +        'name': 'max', 'type': 'integer'}, {'name': 'start', 'type': 'integer'}, {
>>> +        'name': 'cache', 'type': 'integer'}, {'name': 'cycle', 'type': 'boolean'}]]
>>> +  - [288, 1, '_index', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'iid',
>>> +        'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
>>> +        'type': 'string'}, {'name': 'opts', 'type': 'map'}, {'name': 'parts', 'type': 'array'}]]
>>> +  - [289, 1, '_vindex', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'iid',
>>> +        'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
>>> +        'type': 'string'}, {'name': 'opts', 'type': 'map'}, {'name': 'parts', 'type': 'array'}]]
>>> +  - [296, 1, '_func', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
>>> +        'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
>>> +        'type': 'unsigned'}]]
>>> +  - [297, 1, '_vfunc', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
>>> +        'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
>>> +        'type': 'unsigned'}]]
>>> +  - [304, 1, '_user', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
>>> +        'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
>>> +        'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
>>> +  - [305, 1, '_vuser', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
>>> +        'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
>>> +        'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
>>> +  - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
>>> +        'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
>>> +      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
>>> +  - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
>>> +      {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
>>> +      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
>>> +  - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
>>> +        'type': 'string'}]]
>>> +  - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'space_id',
>>> +        'type': 'unsigned'}, {'name': 'opts', 'type': 'map'}]]
>>> +  - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
>>> +        'type': 'unsigned'}]]
>>> +  - [340, 1, '_space_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'},
>>> +      {'name': 'sequence_id', 'type': 'unsigned'}, {'name': 'is_generated', 'type': 'boolean'}]]
>>> +  - [356, 1, '_fk_constraint', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'},
>>> +      {'name': 'child_id', 'type': 'unsigned'}, {'name': 'parent_id', 'type': 'unsigned'},
>>> +      {'name': 'is_deferred', 'type': 'boolean'}, {'name': 'match', 'type': 'string'},
>>> +      {'name': 'on_delete', 'type': 'string'}, {'name': 'on_update', 'type': 'string'},
>>> +      {'name': 'child_cols', 'type': 'array'}, {'name': 'parent_cols', 'type': 'array'}]]
>>
>> 14. Too huge output, which will change on each update in system
>> spaces set and format. We already have similar tests printing
>> everything out, and usually it is not a pleasant business to fix
>> them after each touch of upgrade.lua. I would rather replaced it
>> like this:
>>
>>     box.space._vspace.index[1]:count(1) > 0
> 
> Done.

3. No, it is not. This hunk is still literally absolutely the same.
Please, do it.