From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 211542A81B for ; Wed, 17 Apr 2019 06:11:31 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GsF7B7UC2gpl for ; Wed, 17 Apr 2019 06:11:31 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 465412684D for ; Wed, 17 Apr 2019 06:11:30 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] Set format for spaces with sysview engine References: <265af30c-727f-e781-e2c3-4bc13c98eeae@tarantool.org> <20190417081320.xwcf3ogkvrspgdu3@tarantool.org> From: Vladislav Shpilevoy Message-ID: <68ec9d10-4fa6-4997-54bd-4a80b1ca79ce@tarantool.org> Date: Wed, 17 Apr 2019 13:11:26 +0300 MIME-Version: 1.0 In-Reply-To: <20190417081320.xwcf3ogkvrspgdu3@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Kirill Yukhin Cc: tarantool-patches@freelists.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.