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 BA5C42678B for ; Thu, 23 May 2019 06:41:41 -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 g6QP6ilYyAJC for ; Thu, 23 May 2019 06:41:41 -0400 (EDT) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 7B7F1265C2 for ; Thu, 23 May 2019 06:41:41 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 4/4] box: user-friendly interface to manage ck constraints References: <2c91472289c012378a80a90829ab50dd9fee57f8.1558014727.git.kshcherbatov@tarantool.org> From: Kirill Shcherbatov Message-ID: <55d520b5-40b1-1446-a319-ae2df06f4da1@tarantool.org> Date: Thu, 23 May 2019 13:41:39 +0300 MIME-Version: 1.0 In-Reply-To: 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: tarantool-patches@freelists.org, Vladislav Shpilevoy 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._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!