[tarantool-patches] [PATCH v7 4/6] lua: create method dump_lua for port_sql

[imeevma@tarantool.org]

Hi! Thank you for review! My answers, diff between versions and
new patch below.

On 12/29/18 4:19 PM, Vladislav Shpilevoy wrote:
> 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.
>
Ok, understood.

>> 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.
>
Fixed. Partly fixed in previous patch.

>> +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'.
>
Fixed.

>> + *
>> + * @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).
>
Thanks, fixed.

>> +        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.
>
Fixed.

>> +        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.
>
Fixed, partly in previous patch.

>> +         */
>> +        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?
>
Thanks, fixed.

>> +
>> +    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; 


Diff between versions:

commit 20390a0d00506eb683e0bb6e9144d47acec0937e
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Wed Jan 9 18:35:14 2019 +0300

    Temporary: review fix

diff --git a/src/box/execute.c b/src/box/execute.c
index b8a55be..10b309a 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -147,6 +147,14 @@ static_assert(sizeof(struct port_sql) <= sizeof(struct port),
 static int
 port_sql_dump_msgpack(struct port *port, struct obuf *out);
 
+/**
+ * Dump data from port to Lua stack. Data in port contains tuples,
+ * metadata, or information obtained from an executed SQL query.
+ * The port is destroyed.
+ *
+ * @param port Port that contains SQL response.
+ * @param L Lua stack.
+ */
 static void
 port_sql_dump_lua(struct port *port, struct lua_State *L);
 
@@ -681,7 +689,7 @@ finish:
 }
 
 /**
- * Push a metadata of the prepared statement to Lua stack.
+ * Serialize a description of the prepared statement.
  *
  * @param stmt Prepared statement.
  * @param L Lua stack.
@@ -694,7 +702,7 @@ lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
 	assert(column_count > 0);
 	lua_createtable(L, column_count, 0);
 	for (int i = 0; i < column_count; ++i) {
-		lua_createtable(L, 2, 0);
+		lua_createtable(L, 0, 2);
 		const char *name = sqlite3_column_name(stmt, i);
 		const char *type = sqlite3_column_datatype(stmt, i);
 		/*
@@ -703,6 +711,7 @@ lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
 		 * column_name simply returns them.
 		 */
 		assert(name != NULL);
+		assert(type != NULL);
 		lua_pushstring(L, name);
 		lua_setfield(L, -2, "name");
 		lua_pushstring(L, type);
@@ -711,13 +720,6 @@ lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
 	}
 }
 
-/**
- * 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)
 {
@@ -729,10 +731,6 @@ port_sql_dump_lua(struct port *port, struct lua_State *L)
 		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.
-		 */
 		port_tuple_vtab.dump_lua(port, L);
 		lua_setfield(L, -2, "rows");
 	} else {
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index a55566e..f18a797 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -118,14 +118,12 @@ lbox_execute(struct lua_State *L)
 	int bind_count = 0;
 	size_t length;
 	struct port port;
+	int top = lua_gettop(L);
 
-	if (lua_type(L, 1) != LUA_TSTRING)
+	if (top != 1 || ! lua_isstring(L, 1))
 		return luaL_error(L, "Usage: box.execute(sqlstring)");
 
 	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);


New version:

commit 530bb8859686ce901b98394c4c9a8d23a9524f48
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 a81f482..10b309a 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -43,6 +43,7 @@
 #include "port.h"
 #include "tuple.h"
 #include "sql/vdbe.h"
+#include "lua/utils.h"
 
 const char *sql_type_strs[] = {
 	NULL,
@@ -146,6 +147,17 @@ static_assert(sizeof(struct port_sql) <= sizeof(struct port),
 static int
 port_sql_dump_msgpack(struct port *port, struct obuf *out);
 
+/**
+ * Dump data from port to Lua stack. Data in port contains tuples,
+ * metadata, or information obtained from an executed SQL query.
+ * The port is destroyed.
+ *
+ * @param port Port that contains SQL response.
+ * @param L Lua stack.
+ */
+static void
+port_sql_dump_lua(struct port *port, struct lua_State *L);
+
 static void
 port_sql_destroy(struct port *base)
 {
@@ -156,7 +168,7 @@ port_sql_destroy(struct port *base)
 static 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,
 };
@@ -677,6 +689,77 @@ finish:
 }
 
 /**
+ * Serialize a description of the prepared statement.
+ *
+ * @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, 0, 2);
+		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);
+		assert(type != NULL);
+		lua_pushstring(L, name);
+		lua_setfield(L, -2, "name");
+		lua_pushstring(L, type);
+		lua_setfield(L, -2, "type");
+		lua_rawseti(L, -2, i + 1);
+	}
+}
+
+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");
+		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);
+}
+
+/**
  * Execute prepared SQL statement.
  *
  * This function uses region to allocate memory for temporary
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 6923666..f18a797 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -6,6 +6,7 @@
 #include <info.h>
 #include "lua/info.h"
 #include "lua/utils.h"
+#include "box/execute.h"
 
 static void
 lua_push_column_names(struct lua_State *L, struct sqlite3_stmt *stmt)
@@ -111,6 +112,26 @@ sqlerror:
 }
 
 static int
+lbox_execute(struct lua_State *L)
+{
+	struct sql_bind *bind = NULL;
+	int bind_count = 0;
+	size_t length;
+	struct port port;
+	int top = lua_gettop(L);
+
+	if (top != 1 || ! lua_isstring(L, 1))
+		return luaL_error(L, "Usage: box.execute(sqlstring)");
+
+	const char *sql = lua_tolstring(L, 1, &length);
+	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;
@@ -124,6 +145,7 @@ box_lua_sqlite_init(struct lua_State *L)
 {
 	static const struct luaL_Reg module_funcs [] = {
 		{"execute", lua_sql_execute},
+		{"new_execute", lbox_execute},
 		{"debug", lua_sql_debug},
 		{NULL, NULL}
 	};
-- 
2.7.4