[tarantool-patches] Re: [PATCH v4 4/4] box: user-friendly interface to manage ck constraints

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

Hi! Thank you for the review.

>> the last operation would fail because 1.1 is silently
>> cast to integer 1 which is not greater than 1.
> 
> 1. I do not see that comment on the branch.
...

>> +box.schema.ck_constraint_mt = ck_constraint_mt
> 
> 2. Please, do not expose ck_constraint_mt.
> box.schema.<space/index>_mt are public and documented
> metatables to replace certain DML DQL operations with
> user's one.
> 
> Checks does not provide methods except drop, so it
> makes no sense to make their metatable configurable.
> 
> It means, that you don't need to wrap checks below.
Ok. Done.

>> +/**
>> + * Make ck_constraints available in Lua, via ck_constraint[]
>> + * array.
>> + * Returns a new table representing a space on top of the Lua
>> + * stack.
> 
> 3. Representing a space? How lbox_ck_constraint can return a space?
> As I see, it does not return anything. It just modifies an existing
> space table.
> 
>> + */
>> +static void
>> +lbox_ck_constraint(struct lua_State *L, struct space *space, int i)
Updated comment.

> 
> 4. lbox_ck_constraint_what? If a function is not a getter, it should
> have a verb saying what the function does.
> 
> What is 'i' argument? Index of space table?
This code is all similar to the code that is near it...

> 5. Why do you avoid serialization? What will happen,
> if I will write
> 
>     tarantool> s1.ck_constraint.physics
I don't like space_id to be show. It may differ.

> 
> 6. The test does not check, that after a constraint is
> dropped, a tuple can be inserted violating the dropped
> constraint.
It is not so, actually I test exactly this case.
> 
> Also, I do not see a test, that a user can't insert
> constraints with the same names.
Appended.


Tnx!