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

[Nikita Pettik]

On 16 Sep 16:23, Kirill Shcherbatov wrote:
> 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
> ---
>  src/box/sql/func.c            | 40 +++++++++++++++++++++++------------
>  test/sql-tap/lua_sql.test.lua | 34 +++++++----------------------
>  2 files changed, 35 insertions(+), 39 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 1d8f28a51..b530bb961 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);

If this is a bug, please move it to a separate patch and provide
test case.

>  			mpstream_encode_uint(&stream, val);
>  			break;
>  		}
> @@ -254,19 +255,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:
> @@ -321,10 +335,10 @@ port_tuple_get_vdbemem(struct port *base, uint32_t *size)
>  			sqlVdbeMemSetDouble(&val[i], mp_decode_double(&data));
>  			break;
>  		case MP_INT:
> -			mem_set_i64(val, mp_decode_int(&data));
> +			mem_set_i64(&val[i], mp_decode_int(&data));
>  			break;
>  		case MP_UINT:
> -			mem_set_u64(val, mp_decode_uint(&data));
> +			mem_set_u64(&val[i], mp_decode_uint(&data));
> @@ -333,7 +347,7 @@ port_tuple_get_vdbemem(struct port *base, uint32_t *size)
>  				goto error;
>  			break;
>  		case MP_NIL:
> -			sqlVdbeMemSetNull(val);
> +			sqlVdbeMemSetNull(&val[i]);
>
These two places also look like bugs as well...

> diff --git a/test/sql-tap/lua_sql.test.lua b/test/sql-tap/lua_sql.test.lua
> index 67eff2d1b..50397cf2d 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",
> @@ -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 = {'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?

>                          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})

Btw, is this documented fact that nil is converted to NULL?
So box.NULL and nil means the same in SQL..