[tarantool-patches] Re: [PATCH v2 4/6] lua: add luaT_new_key_def()

  • From: Vladimir Davydov <vdavydov.dev@xxxxxxxxx>
  • To: Alexander Turenko <alexander.turenko@xxxxxxxxxxxxx>
  • Date: Thu, 10 Jan 2019 16:07:56 +0300

On Wed, Jan 09, 2019 at 11:20:12PM +0300, Alexander Turenko wrote:

The function is needed to create the new struct key_def from C code
using a Lua table in the format compatible with
box.space[...].index[...].parts and
net_box_conn.space[...].index[...].parts.

Needed for #3276.
---
 extra/exports                    |   1 +
 src/CMakeLists.txt               |   1 +
 src/box/CMakeLists.txt           |   1 +
 src/box/lua/key_def.c            | 217 +++++++++++++++++++++++++++++++
 src/box/lua/key_def.h            |  61 +++++++++
 test/app-tap/module_api.c        |  13 ++
 test/app-tap/module_api.test.lua |  89 ++++++++++++-
 7 files changed, 381 insertions(+), 2 deletions(-)
 create mode 100644 src/box/lua/key_def.c
 create mode 100644 src/box/lua/key_def.h

diff --git a/extra/exports b/extra/exports
index af6863963..497719ed8 100644
--- a/extra/exports
+++ b/extra/exports
@@ -210,6 +210,7 @@ clock_realtime64
 clock_monotonic64
 clock_process64
 clock_thread64
+luaT_new_key_def
 
 # Lua / LuaJIT
 
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 04de5ad04..494c8d391 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -202,6 +202,7 @@ set(api_headers
     ${CMAKE_SOURCE_DIR}/src/lua/error.h
     ${CMAKE_SOURCE_DIR}/src/box/txn.h
     ${CMAKE_SOURCE_DIR}/src/box/key_def.h
+    ${CMAKE_SOURCE_DIR}/src/box/lua/key_def.h
     ${CMAKE_SOURCE_DIR}/src/box/field_def.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple_format.h
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 5521e489e..0db093768 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -139,6 +139,7 @@ add_library(box STATIC
     lua/net_box.c
     lua/xlog.c
     lua/sql.c
+    lua/key_def.c
     ${bin_sources})
 
 target_link_libraries(box box_error tuple stat xrow xlog vclock crc32 
scramble
diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
new file mode 100644
index 000000000..60247f427
--- /dev/null
+++ b/src/box/lua/key_def.c
@@ -0,0 +1,217 @@
+/*
+ * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include "box/lua/key_def.h"
+
+#include <lua.h>
+#include <lauxlib.h>
+#include "diag.h"
+#include "box/key_def.h"
+#include "box/box.h"
+#include "box/coll_id_cache.h"
+#include "lua/utils.h"
+
+struct key_def *
+luaT_new_key_def(struct lua_State *L, int idx)

If you agree with luaT_tuple_new, then rename this function to
luaT_key_def_new pls.

+{
+     if (lua_istable(L, idx) != 1) {
+             luaL_error(L, "Bad params, use: luaT_new_key_def({"
+                               "{fieldno = fieldno, type = type"
+                               "[, is_nullable = is_nullable"
+                               "[, collation_id = collation_id"

Hm, what's collation_id for?

+                               "[, collation = collation]]]}, ...}");

This looks like you can't specify collation without is_nullable.
Should be

        luaT_new_key_def({{fieldno = FIELDNO, type = TYPE[, is_nullable = true 
| false][, collation = COLLATION]}})

+             unreachable();
+             return NULL;
+     }
+     uint32_t key_parts_count = 0;
+     uint32_t capacity = 8;
+
+     const ssize_t parts_size = sizeof(struct key_part_def) * capacity;

Can't we figure out the table length right away instead of reallocaing
key_part_def array?

+     struct key_part_def *parts = NULL;
+     parts = (struct key_part_def *) malloc(parts_size);
+     if (parts == NULL) {
+             diag_set(OutOfMemory, parts_size, "malloc", "parts");
+             luaT_error(L);
+             unreachable();
+             return NULL;
+     }
+
+     while (true) {

Would be nice to factor out part creation to a separate function.

+             lua_pushinteger(L, key_parts_count + 1);

We would call this variable key_part_count (without 's') or even just
part_count, as you called the array of key parts simply 'parts'.

+             lua_gettable(L, idx);
+             if (lua_isnil(L, -1))
+                     break;
+
+             /* Extend parts if necessary. */
+             if (key_parts_count == capacity) {
+                     capacity *= 2;
+                     struct key_part_def *old_parts = parts;
+                     const ssize_t parts_size =
+                             sizeof(struct key_part_def) * capacity;
+                     parts = (struct key_part_def *) realloc(parts,
+                                                             parts_size);
+                     if (parts == NULL) {
+                             free(old_parts);
+                             diag_set(OutOfMemory, parts_size / 2, "malloc",
+                                      "parts");
+                             luaT_error(L);
+                             unreachable();
+                             return NULL;
+                     }
+             }
+
+             /* Set parts[key_parts_count].fieldno. */
+             lua_pushstring(L, "fieldno");
+             lua_gettable(L, -2);
+             if (lua_isnil(L, -1)) {
+                     free(parts);
+                     luaL_error(L, "fieldno must not be nil");
+                     unreachable();
+                     return NULL;
+             }
+             /*
+              * Transform one-based Lua fieldno to zero-based
+              * fieldno to use in key_def_new().
+              */
+             parts[key_parts_count].fieldno = lua_tointeger(L, -1) - 1;

Use TUPLE_INDEX_BASE instead of 1 pls.

+             lua_pop(L, 1);
+
+             /* Set parts[key_parts_count].type. */
+             lua_pushstring(L, "type");
+             lua_gettable(L, -2);
+             if (lua_isnil(L, -1)) {
+                     free(parts);
+                     luaL_error(L, "type must not be nil");
+                     unreachable();
+                     return NULL;
+             }
+             size_t type_len;
+             const char *type_name = lua_tolstring(L, -1, &type_len);
+             lua_pop(L, 1);
+             parts[key_parts_count].type = field_type_by_name(type_name,
+                                                              type_len);
+             if (parts[key_parts_count].type == field_type_MAX) {
+                     free(parts);
+                     luaL_error(L, "Unknown field type: %s", type_name);
+                     unreachable();
+                     return NULL;
+             }
+
+             /*
+              * Set parts[key_parts_count].is_nullable and
+              * parts[key_parts_count].nullable_action.
+              */
+             lua_pushstring(L, "is_nullable");
+             lua_gettable(L, -2);
+             if (lua_isnil(L, -1)) {
+                     parts[key_parts_count].is_nullable = false;
+                     parts[key_parts_count].nullable_action =
+                             ON_CONFLICT_ACTION_DEFAULT;
+             } else {
+                     parts[key_parts_count].is_nullable =
+                             lua_toboolean(L, -1);
+                     parts[key_parts_count].nullable_action =
+                             ON_CONFLICT_ACTION_NONE;
+             }
+             lua_pop(L, 1);
+
+             /* Set parts[key_parts_count].coll_id using collation_id. */
+             lua_pushstring(L, "collation_id");
+             lua_gettable(L, -2);
+             if (lua_isnil(L, -1))
+                     parts[key_parts_count].coll_id = COLL_NONE;
+             else
+                     parts[key_parts_count].coll_id = lua_tointeger(L, -1);
+             lua_pop(L, 1);
+
+             /* Set parts[key_parts_count].coll_id using collation. */
+             lua_pushstring(L, "collation");
+             lua_gettable(L, -2);
+             /* Check whether box.cfg{} was called. */

Collations should be usable even without box.cfg IIRC. Well, not all of
them I think, but still you don't need to check box.cfg() here AFAIU.

+             if ((parts[key_parts_count].coll_id != COLL_NONE ||
+                 !lua_isnil(L, -1)) && !box_is_configured()) {
+                     free(parts);
+                     luaL_error(L, "Cannot use collations: "
+                                   "please call box.cfg{}");
+                     unreachable();
+                     return NULL;
+             }
+             if (!lua_isnil(L, -1)) {
+                     if (parts[key_parts_count].coll_id != COLL_NONE) {
+                             free(parts);
+                             luaL_error(L, "Conflicting options: "
+                                           "collation_id and collation");
+                             unreachable();
+                             return NULL;
+                     }
+                     size_t coll_name_len;
+                     const char *coll_name = lua_tolstring(L, -1,
+                                                           &coll_name_len);
+                     struct coll_id *coll_id = coll_by_name(coll_name,
+                                                            coll_name_len);

Ouch, this doesn't seem to belong here. Ideally, it should be done by
key_def_new(). Can we rework key_part_def so that it stores collation
string instead of collation id?

+                     if (coll_id == NULL) {
+                             free(parts);
+                             luaL_error(L, "Unknown collation: \"%s\"",
+                                        coll_name);
+                             unreachable();
+                             return NULL;
+                     }
+                     parts[key_parts_count].coll_id = coll_id->id;
+             }
+             lua_pop(L, 1);
+
+             /* Check coll_id. */
+             struct coll_id *coll_id =
+                     coll_by_id(parts[key_parts_count].coll_id);
+             if (parts[key_parts_count].coll_id != COLL_NONE &&
+                 coll_id == NULL) {
+                     uint32_t collation_id = parts[key_parts_count].coll_id;
+                     free(parts);
+                     luaL_error(L, "Unknown collation_id: %d", collation_id);
+                     unreachable();
+                     return NULL;
+             }
+
+             /* Set parts[key_parts_count].sort_order. */
+             parts[key_parts_count].sort_order = SORT_ORDER_ASC;
+
+             ++key_parts_count;
+     }
+
+     struct key_def *key_def = key_def_new(parts, key_parts_count);
+     free(parts);
+     if (key_def == NULL) {
+             luaL_error(L, "Cannot create key_def");
+             unreachable();
+             return NULL;
+     }
+     return key_def;
+}
diff --git a/src/box/lua/key_def.h b/src/box/lua/key_def.h
new file mode 100644
index 000000000..55292fb7e
--- /dev/null
+++ b/src/box/lua/key_def.h
@@ -0,0 +1,61 @@
+#ifndef TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED
+#define TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED
+/*
+ * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct key_def;
+struct lua_State;
+
+/** \cond public */
+
+/**
+ * Create the new key_def from a Lua table.

a key_def

+ *
+ * Expected a table of key parts on the Lua stack. The format is
+ * the same as box.space.<...>.index.<...>.parts or corresponding
+ * net.box's one.
+ *
+ * Returns the new key_def.
+ */
+struct key_def *
+luaT_new_key_def(struct lua_State *L, int idx);
+
+/** \endcond public */
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED */
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index b81a98056..34ab54bc0 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -449,6 +449,18 @@ test_iscallable(lua_State *L)
      return 1;
 }
 
+static int
+test_luaT_new_key_def(lua_State *L)
+{
+     /*
+      * Ignore the return value. Here we test whether the
+      * function raises an error.
+      */
+     luaT_new_key_def(L, 1);

It would be nice to test that it actually creates a valid key_def.
Testing error conditions is less important.

+     lua_pop(L, 1);
+     return 0;
+}
+
 LUA_API int
 luaopen_module_api(lua_State *L)
 {
@@ -477,6 +489,7 @@ luaopen_module_api(lua_State *L)
              {"test_state", test_state},
              {"test_tostring", test_tostring},
              {"iscallable", test_iscallable},
+             {"luaT_new_key_def", test_luaT_new_key_def},
              {NULL, NULL}
      };
      luaL_register(L, "module_api", lib);

Other related posts: