[tarantool-patches] Re: [PATCH v5 3/5] sql: create method dump_lua for port_sql
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Dec 25 17:57:03 MSK 2018
Thanks for the patch! See 7 comments below.
On 22/12/2018 14:31, imeevma at tarantool.org wrote:
> This patch creates the dump_lua method for port_sql. It also
> defines a new function box.sql.new_execute(), which will be
> converted to box.sql.execute().
>
> Part of #3505
> ---
> src/box/execute.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> src/box/lua/sql.c | 25 +++++++++++++++++
> 2 files changed, 108 insertions(+), 1 deletion(-)
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index b07de28..3cbee4f 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -674,6 +675,87 @@ finish:
> return rc;
> }
>
> +/**
> + * Push a metadata of the prepared statement to Lua stack.
> + *
> + * @param stmt Prepared statement.
> + * @param L Lua stack.
> + * @param column_count Statement's column count.
> + */
> +static inline void
> +lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
> + int column_count)
> +{
> + assert(column_count > 0);
> + lua_createtable(L, column_count, 0);
> + for (int i = 0; i < column_count; ++i) {
> + lua_createtable(L, 2, 0);
> + const char *name = sqlite3_column_name(stmt, i);
> + const char *type = sqlite3_column_datatype(stmt, i);
> + /*
> + * Can not fail, since all column names are
> + * preallocated during prepare phase and the
> + * column_name simply returns them.
> + */
> + assert(name != NULL);
> + lua_pushlstring(L, name, strlen(name));
> + lua_setfield(L, -2, "name");
> + lua_pushlstring(L, type, strlen(type));
1. Use lua_pushstring.
> + lua_setfield(L, -2, "type");
> + lua_rawseti(L, -2, i + 1);
> + }
> + lua_setfield(L, -2, "metadata");
2. It is strange, that this function creates a table and sets it
as a member of some externally created table. Please, move this line
to a caller function.
> +}
> +
> +/**
> + * Dump a built response into Lua stack. The response is
> + * destroyed.
> + *
> + * @param port port with EXECUTE response.
> + * @param L Lua stack.
> + */
> +static void
> +port_sql_dump_lua(struct port *port, struct lua_State *L)
> +{
> + assert(port->vtab == &port_sql_vtab);
> + sqlite3 *db = sql_get();
> + struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
> + int column_count = sqlite3_column_count(stmt);
> + if (column_count > 0) {
> + int keys = 2;
> + lua_createtable(L, 0, keys);
3. Just inline keys 2.
> + lua_sql_get_description(stmt, L, column_count);
> + port_tuple_vtab.dump_lua(port, L);
> + lua_setfield(L, -2, "rows");
> + } else {
> + assert(((struct port_tuple *)port)->size == 0);
> + struct stailq *autoinc_id_list =
> + vdbe_autoinc_id_list((struct Vdbe *)stmt);
> +
> + int dynamic_keys = stailq_empty(autoinc_id_list) ? 0 : 1;
> + lua_createtable(L, 1, dynamic_keys);
4. Why 1 array element? And why do you separate fixed/optional keys?
lua_createtable does not take statis/dynamic key count. It takes
array/map key count.
> +
> + luaL_pushuint64(L, db->nChange);
> + lua_setfield(L, -2, "rowcount");
> +
> + if (!stailq_empty(autoinc_id_list)) {
> + lua_newtable(L);
> + int i = 1;
> + struct autoinc_id_entry *id_entry;
> + stailq_foreach_entry(id_entry, autoinc_id_list, link) {
> + if (id_entry->id >= 0)
> + luaL_pushuint64(L, id_entry->id);
> + else
> + luaL_pushint64(L, id_entry->id);
> + lua_rawseti(L, -2, i++);
> + }
> + lua_setfield(L, -2, "autoincrement_ids");
> + }
> + }
> + port_destroy(port);
> + sqlite3_finalize(stmt);
5. I think, port_sql_destroy should call sqlite3_finalize.
> +}
> +
> static void
> port_sql_destroy(struct port *base)
> {
> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
> index 6923666..318f331 100644
> --- a/src/box/lua/sql.c
> +++ b/src/box/lua/sql.c
> @@ -111,6 +112,29 @@ sqlerror:
> }
>
> static int
> +lbox_execute(struct lua_State *L)
> +{
> + struct sql_bind *bind = NULL;
> + int bind_count = 0;
> + size_t length;
> + struct port port;
> +
> + struct sqlite3 *db = sql_get();
> + if (db == NULL)
> + return luaL_error(L, "not ready");
6. Impossible. It isn't?
> +
> + const char *sql = lua_tolstring(L, 1, &length);
> + if (sql == NULL)
> + return luaL_error(L, "usage: box.execute(sqlstring)");
7. Please, start message with a capital letter.
> +
> + if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
> + &fiber()->gc) != 0)
> + return luaT_error(L);
> + port_dump_lua(&port, L);
> + return 1;
> +}
> +
> +static int
> lua_sql_debug(struct lua_State *L)
> {
> struct info_handler info;
More information about the Tarantool-patches
mailing list