[tarantool-patches] Re: [PATCH v2 1/2] Introduce 'view' space option

[n.pettik]

> On 27 Mar 2018, at 09:02, Konstantin Osipov <kostja at tarantool.org> wrote:
> 
> * Nikita Pettik <korablev at tarantool.org> [18/03/27 02:06]:
>> +	if (opts.view && opts.sql == NULL) {
>> +		tnt_raise(ClientError, ER_VIEW_MISSING_SQL);
>> +	}
> 
> We usually do not add {} braces around one-line if bodies.

Fixed:

-       if (opts.view && opts.sql == NULL) {
+       if (opts.is_view && opts.sql == NULL)
                tnt_raise(ClientError, ER_VIEW_MISSING_SQL);
-       }

> 
>> 	struct space_def *def =
>> 		space_def_new_xc(id, uid, exact_field_count, name, name_len,
>> 				 engine_name, engine_name_len, &opts, fields,
>> @@ -1590,6 +1593,10 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
>> 	uint32_t iid = tuple_field_u32_xc(old_tuple ? old_tuple : new_tuple,
>> 					  BOX_INDEX_FIELD_ID);
>> 	struct space *old_space = space_cache_find_xc(id);
>> +	if (old_space->def->opts.view) {
>> +		tnt_raise(ClientError, ER_ALTER_SPACE, space_name(old_space),
>> +			  "can not alter indexes on view");
>> +	}
> 
> can not add index on a view.
> 
> You know for sure it's add, since it can't be drop or modify,
> so let's the error message more specific.
> 
> You can also introduce a separate error code for the purpose,
> ER_ALTER_VIEW.

Fixed:

-       if (old_space->def->opts.view) {
+       if (old_space->def->opts.is_view) {
                tnt_raise(ClientError, ER_ALTER_SPACE, space_name(old_space),
-                         "can not alter indexes on view");
+                         "can not add index on a view");


> 
>> const struct space_opts space_opts_default = {
>> 	/* .temporary = */ false,
>> +	/* .view = */ false,
>> 	/* .sql        = */ NULL,
>> };
> 
> is_view. Usually we add is_  or has_ prefix to boolean state
> variables.

But field ’temporary’ in the same struct isn’t called ‘is_temporary’,
so I just didn’t want to outline this field. Anyway, renamed to ‘is_view’.

> 
>> +	if (new_def->opts.view != old_def->opts.view) {
>> +		diag_set(ClientError, ER_ALTER_SPACE, old_def->name,
>> +			 "can not transform space to view and visa versa");
>> +		return -1;
>> +	}
> 
> can't convert a space to a view or vice vice versa.
> 
> In future, please check all error messages you add with Elena.
> 

Fixed:

-       if (new_def->opts.view != old_def->opts.view) {
+       if (new_def->opts.is_view != old_def->opts.is_view) {
                diag_set(ClientError, ER_ALTER_SPACE, old_def->name,
-                        "can not transform space to view and visa versa");
+                        "can not convert a space to a view and vice versa");