[tarantool-patches] Re: [PATCH v4 4/4] box: user-friendly interface to manage ck constraints
Kirill Shcherbatov
kshcherbatov at tarantool.org
Thu May 23 13:41:39 MSK 2019
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!
More information about the Tarantool-patches
mailing list