[tarantool-patches] Re: [PATCH v6 3/5] lua: create method dump_lua for port_sql

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 6 comments below.

On 28/12/2018 21:11, imeevma@tarantool.org wrote:
> Hi! Thank you for review! My answers, diff between versions and
> new version below.
> 
> On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
>> Thanks for the patch! See 7 comments below.
> 
>> 1. Use lua_pushstring.
> Fixed.
> 
>> 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.
> Fixed.
> 
>> 3. Just inline keys 2.
> Fixed.
> 
>> 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.
> Fixed. Now it takes 1 or 2 as map key count.
> 
>> 5. I think, port_sql_destroy should call sqlite3_finalize.
> Fixed in patch #2.
> 
>> 6. Impossible. It isn't?
> Removed.
> 
>> 7. Please, start message with a capital letter.
> Fixed.

Please, next time write answered inlined into an original
email. With bare comments/answered without a context code it
is hard to remember what was wrong.

> commit 74f30d133b34ea86dc145f6ffee347abfd489cf2
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Fri Dec 21 16:48:37 2018 +0300
> 
>      lua: create method dump_lua for port_sql
>      
>      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
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index c6fcb50..4606239 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -102,12 +103,14 @@ static_assert(sizeof(struct port_sql) <= sizeof(struct port),
>   static int
>   port_sql_dump_msgpack(struct port *port, struct obuf *out);
>   static void
> +port_sql_dump_lua(struct port *port, struct lua_State *L);

1. Please, add empty lines between declarations, and put
a comment above a first function appearance.

> +static void
>   port_sql_destroy(struct port *base);
>   
>   const struct port_vtab port_sql_vtab = {
>   	.dump_msgpack = port_sql_dump_msgpack,
>   	.dump_msgpack_16 = NULL,
> -	.dump_lua = NULL,
> +	.dump_lua = port_sql_dump_lua,
>   	.dump_plain = NULL,
>   	.destroy = port_sql_destroy,
>   };
> @@ -671,6 +674,87 @@ finish:
>   	return rc;
>   }
>   
> +/**
> + * Push a metadata of the prepared statement to Lua stack.

2. Not sure if 'metadata' word is enumerable and can be
accompanied by article 'a'.

> + *
> + * @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);

3. Read carefully what parameters of lua_createtable mean.

First parameter is how many indexed elements preallocate
for indexes [1, N]. Second parameter is how many dictionary
key/value pairs preallocate.

Here table is going to be {name = ..., type = ...} = 2 dictionary
values. So it is lua_createtable(L, 0, 2), not (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);

4. I guess, type != NULL as well.

> +		lua_pushstring(L, name);
> +		lua_setfield(L, -2, "name");
> +		lua_pushstring(L, type);
> +		lua_setfield(L, -2, "type");
> +		lua_rawseti(L, -2, i + 1);
> +	}
> +}
> +
> +/**
> + * 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) {
> +		lua_createtable(L, 0, 2);
> +		lua_sql_get_description(stmt, L, column_count);
> +		lua_setfield(L, -2, "metadata");
> +		/*
> +		 * We can use the port_tuple methods, since
> +		 * port_sql was inherited from it.

5. Drop this comment please, everywhere.

> +		 */
> +		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);
> +		lua_createtable(L, 0, stailq_empty(autoinc_id_list) ? 1 : 2);
> +
> +		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);
> +}
> +
>   static void
>   port_sql_destroy(struct port *base)
>   {
> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
> index 6923666..a55566e 100644
> --- a/src/box/lua/sql.c
> +++ b/src/box/lua/sql.c
> @@ -111,6 +112,28 @@ sqlerror:
>   }
>   
>   static int
> +lbox_execute(struct lua_State *L)
> +{
> +	struct sql_bind *bind = NULL;
> +	int bind_count = 0;
> +	size_t length;
> +	struct port port;
> +
> +	if (lua_type(L, 1) != LUA_TSTRING)
> +		return luaL_error(L, "Usage: box.execute(sqlstring)");

6. Maybe just add here ' || ! lua_isstring(L, 1)' and remove the
check for 'sql == NULL' below?

> +
> +	const char *sql = lua_tolstring(L, 1, &length);
> +	if (sql == NULL)
> +		return luaL_error(L, "Usage: box.execute(sqlstring)");
> +
> +	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;