[tarantool-patches] Re: [PATCH 4/5] sql: make built-ins raise errors for varbin args

[n.pettik]

> On 26 Jul 2019, at 01:03, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> On 24/07/2019 13:42, Nikita Pettik wrote:
>> Since values of type 'varbinary' can't be cast to any other type, let's
>> patch built-in functions which are not assumed to accept arguments of
>> this type to raise an error in case argument turn out to be of type
>> varbinary.
>> 
>> Part of #4206
>> ---
>> src/box/sql/func.c | 24 ++++++++++++++++++++++--
>> 1 file changed, 22 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
>> index 4ec591eee..5fd1496fd 100644
>> --- a/src/box/sql/func.c
>> +++ b/src/box/sql/func.c
>> @@ -506,6 +507,12 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
>> 	}
>> 	if (sql_value_is_null(argv[0]))
>> 		return;
>> +	if (sql_value_type(argv[0]) == MP_BIN) {
>> +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> 
> The patch is ok, but now I see, that we allow 'string' values to functions,
> taking numbers, such as round(). Is it ok?

Yep, there was a discussion (not sure whether it was private or in dev
mailing list) and it was decided that it is ok. Justification is following:
we allow implicit cast between strings and numeric types. In turn,
before passing arguments to functions they should be implicitly casted
if it is required. We don’t add explicit OP_ApplyType opcode, but handle
that conversion inside  each function separately (due to the sqlite legacy).
I guess it would be better to emit extra OP_ApplyType opcode before
each function call, but remove type dispatching from func implementations.