[tarantool-patches] Re: [PATCH v1 2/3] sql: better LUA arguments conversion for UDFs

Kirill Shcherbatov kshcherbatov at tarantool.org
Tue Oct 8 11:38:38 MSK 2019


>>  		case MP_UINT: {
>> -			sql_int64 val = sql_value_int64(param);
>> +			sql_uint64 val = sql_value_uint64(param);
> 
> If this is a bug, please move it to a separate patch and provide
> test case.
> 
>>  			mpstream_encode_uint(&stream, val);

Formally, it is a bug, but nobody can reproduce it. sql_value_int64 and sql_value_uint64 do
exactly the same thing inside while mpstream_encode_uint internally casts val variable
to uint64_t.

I need mention the scenario when this code works: it should be uint64_t passed to
C-language UDF. I've analyzed it carefully, and everything is ok there.
This test case is big enough and is not important: C-language UDFs are already covered
by tests.

I believe that fixing this problem (same as a problem below) silently in scope of "better LUA
arguments conversion for UDFs" is ok.  

>> -			sqlVdbeMemSetNull(val);
>> +			sqlVdbeMemSetNull(&val[i]);
>>
> These two places also look like bugs as well...
UDF function may return multiple values, but we can't handle such situation
normally in SQL. This must be banned. I've included a path that does it in
a separate letter.
>> +                        param_list = {'any'}, returns = 'scalar',
> 
> Why did you have to change types? What is the difference between them,
> if the base type is not map or array?
This doesn't makes sense now. Ok, let's do not change param_list arguments.

> Btw, is this documented fact that nil is converted to NULL?
> So box.NULL and nil means the same in SQL..
This behavior is confirmed as OK by Kostya. I've included  TarantoolBot request
to document it.

=======================================================

Start using comprehensive serializer luaL_tofield() to prepare
LUA arguments for UDFs. This allows to support cdata types
returned from Lua function.

Needed for #4387

@TarantoolBot document
Title: UDF returning nil or box.NULL in SQL

Values nil and box.NULL returned by UDF in SQL
both transformed to SQL NULL and are equal.

Example:
tarantool> box.execute("SELECT LUA('return nil') is NULL
                               and LUA('return nil') is NULL")
---
- metadata:
  - name: LUA('return nil') is NULL and LUA('return nil') is NULL
    type: boolean
  rows:
  - [true]
...
---
 src/box/sql/func.c            | 34 +++++++++++++++++++++++----------
 test/sql-tap/lua_sql.test.lua | 36 +++++++++--------------------------
 2 files changed, 33 insertions(+), 37 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 551613908..04bff5358 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -49,6 +49,7 @@
 #include "box/func.h"
 #include "box/port.h"
 #include "box/tuple.h"
+#include "lua/msgpack.h"
 #include "lua/utils.h"
 #include "mpstream.h"
 
@@ -160,7 +161,7 @@ port_vdbemem_get_msgpack(struct port *base, uint32_t *size)
 			FALLTHROUGH;
 		}
 		case MP_UINT: {
-			sql_int64 val = sql_value_int64(param);
+			sql_uint64 val = sql_value_uint64(param);
 			mpstream_encode_uint(&stream, val);
 			break;
 		}
@@ -255,19 +256,32 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size)
 	if (val == NULL)
 		return NULL;
 	for (int i = 0; i < argc; i++) {
-		switch(lua_type(L, -1 - i)) {
-		case LUA_TBOOLEAN:
-			mem_set_bool(&val[i], lua_toboolean(L, -1 - i));
+		struct luaL_field field;
+		if (luaL_tofield(L, luaL_msgpack_default, -1 - i, &field) < 0)
+			goto error;
+		switch (field.type) {
+		case MP_BOOL:
+			mem_set_bool(&val[i], field.bval);
 			break;
-		case LUA_TNUMBER:
-			sqlVdbeMemSetDouble(&val[i], lua_tonumber(L, -1 - i));
+		case MP_FLOAT:
+			sqlVdbeMemSetDouble(&val[i], field.fval);
 			break;
-		case LUA_TSTRING:
-			if (sqlVdbeMemSetStr(&val[i], lua_tostring(L, -1 - i),
-					     -1, 1, SQL_TRANSIENT) != 0)
+		case MP_DOUBLE:
+			sqlVdbeMemSetDouble(&val[i], field.dval);
+			break;
+		case MP_INT:
+			mem_set_i64(&val[i], field.ival);
+			break;
+		case MP_UINT:
+			mem_set_u64(&val[i], field.ival);
+			break;
+		case MP_STR:
+			if (sqlVdbeMemSetStr(&val[i], field.sval.data,
+					     field.sval.len, 1,
+					     SQL_TRANSIENT) != 0)
 				goto error;
 			break;
-		case LUA_TNIL:
+		case MP_NIL:
 			sqlVdbeMemSetNull(&val[i]);
 			break;
 		default:
diff --git a/test/sql-tap/lua_sql.test.lua b/test/sql-tap/lua_sql.test.lua
index 67eff2d1b..31c563f3a 100755
--- a/test/sql-tap/lua_sql.test.lua
+++ b/test/sql-tap/lua_sql.test.lua
@@ -1,7 +1,7 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
 NULL = require('msgpack').NULL
-test:plan(22)
+test:plan(24)
 
 test:do_test(
     "lua_sql-1.0",
@@ -9,7 +9,7 @@ test:do_test(
         box.schema.func.create('FUNC1', {language = 'Lua',
                          is_deterministic = true,
                          body = 'function(a) return 2 end',
-                         param_list = {'any'}, returns = 'integer',
+                         param_list = {'scalar'}, returns = 'integer',
                          exports = {'LUA', 'SQL'}})
         return test:execsql("select func1(1)")
     end,
@@ -23,7 +23,7 @@ test:do_test(
         box.schema.func.create('FUNC1', {language = 'Lua',
                         is_deterministic = true,
                         body = 'function(a) return a end',
-                        param_list = {'scalar'}, returns = 'integer',
+                        param_list = {'scalar'}, returns = 'scalar',
                         exports = {'LUA', 'SQL'}})
         return test:execsql("select func1(1)")
     end,
@@ -88,6 +88,7 @@ from_lua_to_sql = {
     [3] = {"'1'", "1"},
     [4] = {"true", true},
     [5] = {"false", false},
+    [6] = {12, 12LL},
 }
 
 box.schema.func.create('CHECK_FROM_LUA_TO_SQL', {language = 'Lua',
@@ -97,38 +98,20 @@ box.schema.func.create('CHECK_FROM_LUA_TO_SQL', {language = 'Lua',
                                return from_lua_to_sql[i][2]
                            end
                        ]],
-                       param_list = {'integer'}, returns = 'scalar',
+                       param_list = {'integer'}, returns = 'any',
                        exports = {'LUA', 'SQL'}})
 
 -- check for different types
 for i = 1, #from_lua_to_sql, 1 do
     test:do_execsql_test(
         "lua_sql-2.3."..i,
-        "select "..from_lua_to_sql[i][1].." = check_from_lua_to_sql("..i..")",
+        "select "..tostring(from_lua_to_sql[i][1]).." = check_from_lua_to_sql("..i..")",
         {true})
 end
 
-from_lua_to_sql_bad = {
-    [1] = NULL,
-    [2] = 12LL, -- it is possible to support this type
-}
-
-box.schema.func.create('CHECK_FROM_LUA_TO_SQL_BAD', {language = 'Lua',
-                       is_deterministic = true,
-                       body = [[
-                           function(i)
-                               return from_lua_to_sql_bad[i]
-                           end
-                       ]],
-                       param_list = {'integer'}, returns = 'scalar',
-                       exports = {'LUA', 'SQL'}})
-
-for i = 1, #from_lua_to_sql_bad, 1 do
-    test:do_catchsql_test(
-        "lua_sql-2.5."..i,
-        "select check_from_lua_to_sql_bad("..i..")",
-        {1, "/Unsupported/"})
-end
+test:do_execsql_test("lua_sql-2.4.1", "SELECT LUA('return box.NULL') is NULL", {true})
+test:do_execsql_test("lua_sql-2.4.2", "SELECT LUA('return nil') is NULL", {true})
+test:do_execsql_test("lua_sql-2.4.3", "SELECT LUA('ffi = require(\"ffi\") return ffi.new(\"uint64_t\", (2LLU^64)-1)') = 18446744073709551615", {true})
 
 box.schema.func.create('ALLWAYS_ERROR', {language = 'Lua',
                        is_deterministic = true,
@@ -150,7 +133,6 @@ test:do_catchsql_test(
 box.func.FUNC1:drop()
 box.func.CHECK_FROM_SQL_TO_LUA:drop()
 box.func.CHECK_FROM_LUA_TO_SQL:drop()
-box.func.CHECK_FROM_LUA_TO_SQL_BAD:drop()
 box.func.ALLWAYS_ERROR:drop()
 
 test:finish_test()
-- 
2.23.0





More information about the Tarantool-patches mailing list