[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