Re: [Tarantool-patches] [PATCH v1 1/1] sql: check arguments types of built-in functions

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for the review! I send this patch to Nikita.

My answers and diff below.

On Thu, Oct 15, 2020 at 11:23:00PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> On 14.10.2020 00:44, Mergen Imeev wrote:
> > Hi! Thank you for the review! My answers, diff and new patch below.
> > 
> > On Fri, Oct 09, 2020 at 11:08:32PM +0200, Vladislav Shpilevoy wrote:
> >> Hi! Thanks for the patch!
> >>
> >> See 12 comments/questions below.
> >>
> >> 1.
> >>
> >> 	tarantool> box.execute("SELECT GREATEST(1, '2')")
> >> 	---
> >> 	- metadata:
> >> 	  - name: COLUMN_1
> >> 	    type: scalar
> >> 	  rows:
> >> 	  - ['2']
> >> 	...
> >>
> >> Is it supposed to happen? Shouldn't the function check that all
> >> arguments are of the same type? The same for LEAST().
> >>
> > At the moment LEAST() ang GREATEST() works using SCALAR rules from BOX for
> > comparison. Not sure if this is right, though.
> 
> Isn't this patch aimed to fix such places?
> 
No, as far as I know comparison rules for this functions are not defined.
However, I thought that applying SCALAR rules here is quite good
decision. Not sure though.

> >>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> >>> index 0aedb2d3d..d348c3d09 100644
> >>> --- a/src/box/sql/func.c
> >>> +++ b/src/box/sql/func.c
> >>> @@ -467,30 +467,21 @@ lengthFunc(sql_context * context, int argc, sql_value ** argv)
> >>>  
> >>>  	assert(argc == 1);
> >>>  	UNUSED_PARAMETER(argc);
> >>> -	switch (sql_value_type(argv[0])) {
> >>> -	case MP_BIN:
> >>> -	case MP_ARRAY:
> >>> -	case MP_MAP:
> >>> -	case MP_INT:
> >>> -	case MP_UINT:
> >>> -	case MP_BOOL:
> >>> -	case MP_DOUBLE:{
> >>> -			sql_result_uint(context, sql_value_bytes(argv[0]));
> >>> -			break;
> >>> -		}
> >>> -	case MP_STR:{
> >>> -			const unsigned char *z = sql_value_text(argv[0]);
> >>> -			if (z == 0)
> >>> -				return;
> >>> -			len = sql_utf8_char_count(z, sql_value_bytes(argv[0]));
> >>> -			sql_result_uint(context, len);
> >>> -			break;
> >>> -		}
> >>> -	default:{
> >>> -			sql_result_null(context);
> >>> -			break;
> >>> -		}
> >>> -	}
> >>> +	enum mp_type type = mem_mp_type(argv[0]);
> >>> +	if (type == MP_NIL)
> >>> +		return sql_result_null(context);
> >>> +	if (type == MP_STR) {
> >>> +		const unsigned char *z = sql_value_text(argv[0]);
> >>> +		if (z == NULL)
> >>> +			return sql_result_null(context);
> >>> +		len = sql_utf8_char_count(z, sql_value_bytes(argv[0]));
> >>> +		return sql_result_uint(context, len);
> >>> +	}
> >>> +	if (type == MP_BIN)
> >>> +		return sql_result_uint(context, sql_value_bytes(argv[0]));
> >>> +	diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> >>> +		 sql_value_to_diag_str(argv[0]), "string or varbinary");
> >>> +	context->is_aborted = true;
> >>
> >> 6. What was wrong with leaving it a switch-case? Here and in other places.
> >> For more than 2 similarly looking checks switch usually looks better, and
> >> probably works better as well (but this I don't know for sure, I just
> >> read some stuff about comilers being able to generate kind of smart goto
> >> table for swithches).
> >>
> > Mostly because shich can be used only in functions that accepts only one
> > argument.
> 
> Not exactly. As I see, mostly when you accept multiple arguments, they have
> the same type. So you could switch-test one of them, and the compare == to
> others.
> 
I still believe that in near future all these checks will be removed from these
functions. All these checks will be executed by OP_ApplyType, so the more they
will be similar to each other, the easier it will be to remove them. Just my
thoughts.

> > I think it is better that all of built-in functions (except typeof())
> > have the same structure. Just my thoughts, though.
> 
> Whatever, leave it as you want. Personally I would chose switch where
> possible, but no strict rules here.
> 
> >>> @@ -988,9 +979,9 @@ randomBlob(sql_context * context, int argc, sql_value ** argv)
> >>>  	unsigned char *p;
> >>>  	assert(argc == 1);
> >>>  	UNUSED_PARAMETER(argc);
> >>> -	if (mp_type_is_bloblike(sql_value_type(argv[0]))) {
> >>> +	if (!mp_type_is_numeric(sql_value_type(argv[0]))) {
> >>>  		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> >>> -			 sql_value_to_diag_str(argv[0]), "numeric");
> >>> +			 sql_value_to_diag_str(argv[0]), "integer");
> >>
> >> 9. You check for numeric, but print integer? So if I pass 1.1 it will
> >> pass, and will return 1 random byte. But should throw an error. Is there
> >> a test on that?
> >>
> > According to our implicit cast rules, numbers of all types may be converted to
> > INTEGER. However, currently these functions cannot fully work with our INTEGER
> > type. They may only wor with int64. This also will be mentioned in the issue.
> 
> What issue?
> 
I mean this issue:

>> I plan to create an issue. In this issue I plan to suggest to rework built-in
>> functions so they could work right according to ANSI. Defining argument types
>> will be part of the issue.

I haven't filled this issue fow now. I will do this after this patch is pushed
to master.

> See 9 comments below. Please, send next version to Nikita. We need to
> hurry up, and I am sure he has a lot to add.
> 
> > From d1a97a66e58ad09aa9b1f7dea02b42f5d560c0bc Mon Sep 17 00:00:00 2001
> > From: Mergen Imeev <imeevma@gmail.com>
> > Date: Thu, 8 Oct 2020 00:23:18 +0300
> > Subject: [PATCH] sql: check arguments types of built-in functions
> > 
> > After this patch, the argument types of the SQL built-in functions will
> > be checked. Implicit casting rules will be applied during this check.
> > 
> > Closes #4159
> > 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index 0aedb2d3d..0da6c8f06 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -467,30 +467,21 @@ lengthFunc(sql_context * context, int argc, sql_value ** argv)
> >  
> >  	assert(argc == 1);
> >  	UNUSED_PARAMETER(argc);
> > -	switch (sql_value_type(argv[0])) {
> > -	case MP_BIN:
> > -	case MP_ARRAY:
> > -	case MP_MAP:
> > -	case MP_INT:
> > -	case MP_UINT:
> > -	case MP_BOOL:
> > -	case MP_DOUBLE:{
> > -			sql_result_uint(context, sql_value_bytes(argv[0]));
> > -			break;
> > -		}
> > -	case MP_STR:{
> > -			const unsigned char *z = sql_value_text(argv[0]);
> > -			if (z == 0)
> > -				return;
> > -			len = sql_utf8_char_count(z, sql_value_bytes(argv[0]));
> > -			sql_result_uint(context, len);
> > -			break;
> > -		}
> > -	default:{
> > -			sql_result_null(context);
> > -			break;
> > -		}
> > -	}
> > +	enum mp_type type = mem_mp_type(argv[0]);
> > +	if (type == MP_NIL)
> > +		return sql_result_null(context);
> > +	if (type == MP_STR) {
> > +		const unsigned char *z = sql_value_text(argv[0]);
> > +		if (z == NULL)
> > +			return sql_result_null(context);
> > +		len = sql_utf8_char_count(z, sql_value_bytes(argv[0]));
> > +		return sql_result_uint(context, len);
> > +	}
> > +	if (type == MP_BIN)
> > +		return sql_result_uint(context, sql_value_bytes(argv[0]));
> > +	diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +		 sql_value_to_diag_str(argv[0]), "string or varbinary");
> > +	context->is_aborted = true;
> >  }
> >  
> >  /*
> > @@ -504,44 +495,24 @@ absFunc(sql_context * context, int argc, sql_value ** argv)
> >  {
> >  	assert(argc == 1);
> >  	UNUSED_PARAMETER(argc);
> > -	switch (sql_value_type(argv[0])) {
> > -	case MP_UINT: {
> > -		sql_result_uint(context, sql_value_uint64(argv[0]));
> > -		break;
> > -	}
> > -	case MP_INT: {
> > +	enum mp_type type = mem_mp_type(argv[0]);
> > +	if (type == MP_NIL)
> > +		return sql_result_null(context);
> > +	if (type == MP_UINT)
> > +		return sql_result_uint(context, sql_value_uint64(argv[0]));
> > +	if (type == MP_INT) {
> >  		int64_t value = sql_value_int64(argv[0]);
> > -		assert(value < 0);
> > -		sql_result_uint(context, -value);
> > -		break;
> > -	}
> > -	case MP_NIL:{
> > -			/* IMP: R-37434-19929 Abs(X) returns NULL if X is NULL. */
> > -			sql_result_null(context);
> > -			break;
> > -		}
> > -	case MP_BOOL:
> > -	case MP_BIN:
> > -	case MP_ARRAY:
> > -	case MP_MAP: {
> > -		diag_set(ClientError, ER_INCONSISTENT_TYPES, "number",
> > -			 mem_type_to_str(argv[0]));
> > -		context->is_aborted = true;
> > -		return;
> > +		return sql_result_uint(context, (uint64_t)(-value));
> >  	}
> > -	default:{
> > -			/* Because sql_value_double() returns 0.0 if the argument is not
> > -			 * something that can be converted into a number, we have:
> > -			 * IMP: R-01992-00519 Abs(X) returns 0.0 if X is a string or blob
> > -			 * that cannot be converted to a numeric value.
> > -			 */
> > -			double rVal = sql_value_double(argv[0]);
> > -			if (rVal < 0)
> > -				rVal = -rVal;
> > -			sql_result_double(context, rVal);
> > -			break;
> > -		}
> > +	if (type == MP_DOUBLE) {
> > +		double value = sql_value_double(argv[0]);
> > +		if (value < 0)
> > +			value = -value;
> > +		return sql_result_double(context, value);
> >  	}
> > +	diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +		 sql_value_to_diag_str(argv[0]), "number");
> > +	context->is_aborted = true;
> >  }
> >  
> >  /**
> > @@ -564,34 +535,31 @@ position_func(struct sql_context *context, int argc, struct Mem **argv)
> >  	enum mp_type needle_type = sql_value_type(needle);
> >  	enum mp_type haystack_type = sql_value_type(haystack);
> >  
> > -	if (haystack_type == MP_NIL || needle_type == MP_NIL)
> > -		return;
> > -	/*
> > -	 * Position function can be called only with string
> > -	 * or blob params.
> > -	 */
> > -	struct Mem *inconsistent_type_arg = NULL;
> > -	if (needle_type != MP_STR && needle_type != MP_BIN)
> > -		inconsistent_type_arg = needle;
> > -	if (haystack_type != MP_STR && haystack_type != MP_BIN)
> > -		inconsistent_type_arg = haystack;
> > -	if (inconsistent_type_arg != NULL) {
> > -		diag_set(ClientError, ER_INCONSISTENT_TYPES,
> > -			 "text or varbinary",
> > -			 mem_type_to_str(inconsistent_type_arg));
> > +	if (needle_type != MP_NIL && needle_type != MP_STR &&
> > +	    needle_type != MP_BIN) {
> > +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +			 sql_value_to_diag_str(needle), "string or varbinary");
> >  		context->is_aborted = true;
> >  		return;
> >  	}
> > -	/*
> > -	 * Both params of Position function must be of the same
> > -	 * type.
> > -	 */
> > -	if (haystack_type != needle_type) {
> > -		diag_set(ClientError, ER_INCONSISTENT_TYPES,
> > -			 mem_type_to_str(needle), mem_type_to_str(haystack));
> > -		context->is_aborted = true;
> > -		return;
> > +	if (haystack_type != MP_NIL && haystack_type != needle_type) {
> > +		if (needle_type != MP_NIL) {
> > +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +				 sql_value_to_diag_str(haystack),
> > +				 mem_type_to_str(needle));
> > +			context->is_aborted = true;
> > +			return;
> > +		}
> > +		if (haystack_type != MP_STR && haystack_type != MP_BIN) {
> > +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +				 sql_value_to_diag_str(haystack),
> > +				 "string or varbinary");
> > +			context->is_aborted = true;
> > +			return;
> > +		}
> >  	}
> > +	if (needle_type == MP_NIL || haystack_type == MP_NIL)
> > +		return;
> >  
> >  	int n_needle_bytes = sql_value_bytes(needle);
> >  	int n_haystack_bytes = sql_value_bytes(haystack);
> > @@ -707,6 +675,17 @@ printfFunc(sql_context * context, int argc, sql_value ** argv)
> >  	}
> >  }
> >  
> > +static bool
> > +is_num_to_int_possible(struct Mem *mem)
> 
> 1. The function lacks a comment. Why is it allowed to convert a double
> to an int anywhere? Need to mention that this is the allowed implicit
> cast.
> 
Added a comment. Also moved mp_type_is_numeric() check here and renamed this
function.

> > +{
> > +	enum mp_type type = mem_mp_type(mem);
> > +	assert(mp_type_is_numeric(type));
> > +	if (type == MP_INT || type == MP_UINT)
> > +		return true;
> > +	assert(type == MP_DOUBLE);
> > +	return mem->u.r >= (double)INT64_MIN && mem->u.r < (double)UINT64_MAX;
> 
> 2. Why not <= UINT64_MAX, why <?
> 
Because (double)UINT64_MAX == 2^64, which is not part INTEGER.

> Also would be better to handle such checks inside sql_value_int(). It
> already can return a proper success status from sqlVdbeIntValue(), but
> ignores its result. Maybe not a part of this patchset though due to
> lack of time.
> 
I agree that it is better to fix later, since we need to fix all functions that
receive integer values through this function.

> > @@ -735,22 +713,45 @@ substrFunc(sql_context * context, int argc, sql_value ** argv)
> >  		context->is_aborted = true;
> >  		return;
> >  	}
> > -	if (sql_value_is_null(argv[1])
> > -	    || (argc == 3 && sql_value_is_null(argv[2]))
> > -	    ) {
> > +	enum mp_type type0 = mem_mp_type(argv[0]);
> > +	if (type0 != MP_NIL && type0 != MP_STR && type0 != MP_BIN) {
> > +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +			 sql_value_to_diag_str(argv[0]), "string or varbinary");
> > +		context->is_aborted = true;
> >  		return;
> >  	}
> > -	p0type = sql_value_type(argv[0]);
> > +	enum mp_type type1 = mem_mp_type(argv[1]);
> > +	if (type1 != MP_NIL && (!mp_type_is_numeric(type1) ||
> > +				!is_num_to_int_possible(argv[1]))) {
> > +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +			 sql_value_to_diag_str(argv[1]), "integer");
> > +		context->is_aborted = true;
> > +		return;
> > +	}
> > +	enum mp_type type2 = MP_UINT;
> > +	if (argc == 3) {
> > +		type2 = mem_mp_type(argv[2]);
> > +		if (type2 != MP_NIL && (!mp_type_is_numeric(type2) ||
> > +					!is_num_to_int_possible(argv[2]))) {
> 
> 3. This construction 'is_numeric && int_possible' is repeated
> 6 times in this file. Maybe time to extract into a new function.
> You could just move mp_type_is_numeric() into is_num_to_int_possible(),
> for example.
> 
Fixed. Moved to is_num_to_int_possible().

> > +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +				 sql_value_to_diag_str(argv[2]), "integer");
> > +			context->is_aborted = true;
> > +			return;
> > +		}
> > +	}
> > +	if (type0 == MP_NIL || type1 == MP_NIL || type2 == MP_NIL)
> > +		return;
> > +
> >  	p1 = sql_value_int(argv[1]);
> 
> 4. is_num_to_int_possible() allows values > INT64_MAX, so it will
> overflow here.
> 
True, however it works a bit differently than usual overflow. This should be
fixed in all places sql_value_int(), but I suggest to do this as part of a
different issue, for example one I mentioned above (which is not created
for now, though).

> > -	if (p0type == MP_BIN) {
> > +	if (type0 == MP_BIN) {
> >  		len = sql_value_bytes(argv[0]);
> >  		z = sql_value_blob(argv[0]);
> > -		if (z == 0)
> > +		if (z == NULL)
> >  			return;
> >  		assert(len == sql_value_bytes(argv[0]));
> >  	} else {
> >  		z = sql_value_text(argv[0]);
> > -		if (z == 0)
> > +		if (z == NULL)
> >  			return;
> >  		len = 0;
> >  		if (p1 < 0)
> > @@ -763,9 +764,9 @@ substrFunc(sql_context * context, int argc, sql_value ** argv)
> >  			negP2 = 1;
> >  		}
> >  	} else {
> > -		p2 = sql_context_db_handle(context)->
> > -		    aLimit[SQL_LIMIT_LENGTH];
> > +		p2 = sql_context_db_handle(context)->aLimit[SQL_LIMIT_LENGTH];
> >  	}
> > +
> 
> 5. This whole diff hunk and the 2 changes above it are not necessary.
> 
Removed.

> >  	if (p1 < 0) {
> >  		p1 += len;
> >  		if (p1 < 0) {
> > @@ -836,22 +837,32 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
> >  		context->is_aborted = true;
> >  		return;
> >  	}
> > +	enum mp_type type0 = mem_mp_type(argv[0]);
> > +	if (type0!= MP_NIL && !mp_type_is_numeric(type0)) {
> 
> 6. Missing whitespace after type0.
> 
Fixed.

> > +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +			 sql_value_to_diag_str(argv[0]), "numeric");
> > +		context->is_aborted = true;
> > +		return;
> > +	}
> > +	enum mp_type type1 = MP_UINT;
> >  	if (argc == 2) {
> > -		if (sql_value_is_null(argv[1]))
> > +		type1 = mem_mp_type(argv[1]);
> > +		if (type1 != MP_NIL && (!mp_type_is_numeric(type1) ||
> > +					!is_num_to_int_possible(argv[1]))) {
> > +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +				 sql_value_to_diag_str(argv[1]), "integer");
> > +			context->is_aborted = true;
> >  			return;
> > +		}
> > +	}
> > +	if (type0 == MP_NIL || type1 == MP_NIL)
> > +		return;
> > +
> > +	if (argc == 2) {
> >  		n = sql_value_int(argv[1]);
> >  		if (n < 0)
> >  			n = 0;
> >  	}
> > -	if (sql_value_is_null(argv[0]))
> > -		return;
> > -	enum mp_type mp_type = sql_value_type(argv[0]);
> > -	if (mp_type_is_bloblike(mp_type)) {
> > -		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > -			 sql_value_to_diag_str(argv[0]), "numeric");
> > -		context->is_aborted = true;
> > -		return;
> > -	}
> >  	r = sql_value_double(argv[0]);
> >  	/* If Y==0 and X will fit in a 64-bit int,
> >  	 * handle the rounding directly,
> > @@ -907,9 +918,11 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv)   \
> >  	int n;                                                                 \
> >  	UNUSED_PARAMETER(argc);                                                \
> >  	int arg_type = sql_value_type(argv[0]);                                \
> > -	if (mp_type_is_bloblike(arg_type)) {                                   \
> > -		diag_set(ClientError, ER_INCONSISTENT_TYPES, "text",           \
> > -			 "varbinary");                                         \
> > +	if (arg_type == MP_NIL)                                                \
> > +		return;                                                        \
> > +	if (arg_type != MP_STR) {                                              \
> 
> 7. I would better use a single 'if' for != MP_STR, and would check for
> == MP_NIL inside it. Because almost always the value != MP_NIL, and
> therefore we would avoid unnecessary branching on the common path. Up
> to you.
> 
Fixed here and in one more place.

> > +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,                    \
> > +			 sql_value_to_diag_str(argv[0]), "string");            \
> >  		context->is_aborted = true;                                    \
> >  		return;                                                        \
> >  	}                                                                      \
> > @@ -1750,6 +1846,12 @@ trim_func_one_arg(struct sql_context *context, sql_value *arg)
> >  	enum mp_type val_type = sql_value_type(arg);
> >  	if (val_type == MP_NIL)
> >  		return;
> > +	if (val_type != MP_STR && !mp_type_is_bloblike(val_type)) {
> 
> 8. mp_type_is_bloblike() is true for arrays and maps. Does not look like
> a good check for TRIM function. The same for other usages of this func.
> 
Fixed in all places in this file.

> > +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +			 sql_value_to_diag_str(arg), "string or varbinary");
> > +		context->is_aborted = true;
> > +		return;
> > +	}
> > diff --git a/test/sql-tap/position.test.lua b/test/sql-tap/position.test.lua
> > index e0455abc9..0a2c42173 100755
> > --- a/test/sql-tap/position.test.lua
> > +++ b/test/sql-tap/position.test.lua
> > @@ -238,7 +238,7 @@ test:do_test(
> >          return test:catchsql "SELECT position(34, 123456.78);"
> >      end, {
> >          -- <position-1.24>
> > -        1, "Inconsistent types: expected text or varbinary got real"
> > +        1, "Type mismatch: can not convert 34 to string or varbinary"
> 
> 9. Tbh, the old error looked better. The new one is strange, because 34
> can be converted to a string. It just does not happen implicitly. So the
> message is misleading.
Agree, but this is how error looks in all other places where such implicit case
is disallowed. I think we should fix this error, but not in this issue.


Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 0da6c8f06..4d79c6cb6 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -675,11 +675,16 @@ printfFunc(sql_context * context, int argc, sql_value ** argv)
 	}
 }
 
+/*
+ * Make sure you can get INTEGER values from Mem according to implicit casting
+ * rules. Currently, only numbers can be converted to INTEGER.
+ */
 static bool
-is_num_to_int_possible(struct Mem *mem)
+is_get_int_possible(struct Mem *mem)
 {
 	enum mp_type type = mem_mp_type(mem);
-	assert(mp_type_is_numeric(type));
+	if (!mp_type_is_numeric(type))
+		return false;
 	if (type == MP_INT || type == MP_UINT)
 		return true;
 	assert(type == MP_DOUBLE);
@@ -721,8 +726,7 @@ substrFunc(sql_context * context, int argc, sql_value ** argv)
 		return;
 	}
 	enum mp_type type1 = mem_mp_type(argv[1]);
-	if (type1 != MP_NIL && (!mp_type_is_numeric(type1) ||
-				!is_num_to_int_possible(argv[1]))) {
+	if (type1 != MP_NIL && (!is_get_int_possible(argv[1]))) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 sql_value_to_diag_str(argv[1]), "integer");
 		context->is_aborted = true;
@@ -731,8 +735,7 @@ substrFunc(sql_context * context, int argc, sql_value ** argv)
 	enum mp_type type2 = MP_UINT;
 	if (argc == 3) {
 		type2 = mem_mp_type(argv[2]);
-		if (type2 != MP_NIL && (!mp_type_is_numeric(type2) ||
-					!is_num_to_int_possible(argv[2]))) {
+		if (type2 != MP_NIL && (!is_get_int_possible(argv[2]))) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 				 sql_value_to_diag_str(argv[2]), "integer");
 			context->is_aborted = true;
@@ -746,12 +749,12 @@ substrFunc(sql_context * context, int argc, sql_value ** argv)
 	if (type0 == MP_BIN) {
 		len = sql_value_bytes(argv[0]);
 		z = sql_value_blob(argv[0]);
-		if (z == NULL)
+		if (z == 0)
 			return;
 		assert(len == sql_value_bytes(argv[0]));
 	} else {
 		z = sql_value_text(argv[0]);
-		if (z == NULL)
+		if (z == 0)
 			return;
 		len = 0;
 		if (p1 < 0)
@@ -764,9 +767,9 @@ substrFunc(sql_context * context, int argc, sql_value ** argv)
 			negP2 = 1;
 		}
 	} else {
-		p2 = sql_context_db_handle(context)->aLimit[SQL_LIMIT_LENGTH];
+		p2 = sql_context_db_handle(context)->
+		    aLimit[SQL_LIMIT_LENGTH];
 	}
-
 	if (p1 < 0) {
 		p1 += len;
 		if (p1 < 0) {
@@ -838,7 +841,7 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
 		return;
 	}
 	enum mp_type type0 = mem_mp_type(argv[0]);
-	if (type0!= MP_NIL && !mp_type_is_numeric(type0)) {
+	if (type0 != MP_NIL && !mp_type_is_numeric(type0)) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 sql_value_to_diag_str(argv[0]), "numeric");
 		context->is_aborted = true;
@@ -847,8 +850,7 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
 	enum mp_type type1 = MP_UINT;
 	if (argc == 2) {
 		type1 = mem_mp_type(argv[1]);
-		if (type1 != MP_NIL && (!mp_type_is_numeric(type1) ||
-					!is_num_to_int_possible(argv[1]))) {
+		if (type1 != MP_NIL && (!is_get_int_possible(argv[1]))) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 				 sql_value_to_diag_str(argv[1]), "integer");
 			context->is_aborted = true;
@@ -918,9 +920,9 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv)   \
 	int n;                                                                 \
 	UNUSED_PARAMETER(argc);                                                \
 	int arg_type = sql_value_type(argv[0]);                                \
-	if (arg_type == MP_NIL)                                                \
-		return;                                                        \
 	if (arg_type != MP_STR) {                                              \
+		if (arg_type == MP_NIL)                                        \
+			return;                                                \
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,                    \
 			 sql_value_to_diag_str(argv[0]), "string");            \
 		context->is_aborted = true;                                    \
@@ -1003,8 +1005,7 @@ randomBlob(sql_context * context, int argc, sql_value ** argv)
 	UNUSED_PARAMETER(argc);
 	if (sql_value_is_null(argv[0]))
 		return;
-	if (!mp_type_is_numeric(mem_mp_type(argv[0])) ||
-	    !is_num_to_int_possible(argv[0])) {
+	if (!is_get_int_possible(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 sql_value_to_diag_str(argv[0]), "integer");
 		context->is_aborted = true;
@@ -1506,8 +1507,7 @@ charFunc(sql_context * context, int argc, sql_value ** argv)
 		unsigned c;
 		if (sql_value_is_null(argv[i]))
 			continue;
-		if (!mp_type_is_numeric(mem_mp_type(argv[i])) ||
-		    !is_num_to_int_possible(argv[i])) {
+		if (!is_get_int_possible(argv[i])) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 				 sql_value_to_diag_str(argv[i]), "integer");
 			context->is_aborted = true;
@@ -1584,8 +1584,7 @@ zeroblobFunc(sql_context * context, int argc, sql_value ** argv)
 	UNUSED_PARAMETER(argc);
 	if (sql_value_is_null(argv[0]))
 		return;
-	if (!mp_type_is_numeric(mem_mp_type(argv[0])) ||
-	    !is_num_to_int_possible(argv[0])) {
+	if (!is_get_int_possible(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 sql_value_to_diag_str(argv[0]), "integer");
 		context->is_aborted = true;
@@ -1844,15 +1843,15 @@ trim_func_one_arg(struct sql_context *context, sql_value *arg)
 	/* In case of VARBINARY type default trim octet is X'00'. */
 	const unsigned char *default_trim;
 	enum mp_type val_type = sql_value_type(arg);
-	if (val_type == MP_NIL)
-		return;
-	if (val_type != MP_STR && !mp_type_is_bloblike(val_type)) {
+	if (val_type != MP_STR && val_type != MP_BIN) {
+		if (val_type == MP_NIL)
+			return;
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 sql_value_to_diag_str(arg), "string or varbinary");
 		context->is_aborted = true;
 		return;
 	}
-	if (mp_type_is_bloblike(val_type))
+	if (val_type == MP_BIN)
 		default_trim = (const unsigned char *) "\0";
 	else
 		default_trim = (const unsigned char *) " ";
@@ -1880,7 +1879,7 @@ trim_func_two_args(struct sql_context *context, sql_value *arg1,
 {
 	const unsigned char *input_str, *trim_set;
 	enum mp_type type2 = sql_value_type(arg2);
-	if (type2 != MP_NIL && type2 != MP_STR && !mp_type_is_bloblike(type2)) {
+	if (type2 != MP_NIL && type2 != MP_STR && type2 != MP_BIN) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 sql_value_to_diag_str(arg2), "string or varbinary");
 		context->is_aborted = true;
@@ -1898,7 +1897,7 @@ trim_func_two_args(struct sql_context *context, sql_value *arg1,
 			       input_str, input_str_sz);
 		return;
 	}
-	if (type1 != MP_NIL && type1 != MP_STR && !mp_type_is_bloblike(type1)) {
+	if (type1 != MP_NIL && type1 != MP_STR && type1 != MP_BIN) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 sql_value_to_diag_str(arg1), "string or varbinary");
 		context->is_aborted = true;
@@ -1935,14 +1934,14 @@ trim_func_three_args(struct sql_context *context, sql_value *arg1,
 {
 	assert(sql_value_type(arg1) == MP_INT || sql_value_type(arg1) == MP_UINT);
 	enum mp_type type2 = sql_value_type(arg2);
-	if (type2 != MP_NIL && type2 != MP_STR && !mp_type_is_bloblike(type2)) {
+	if (type2 != MP_NIL && type2 != MP_STR && type2 != MP_BIN) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 sql_value_to_diag_str(arg2), "string or varbinary");
 		context->is_aborted = true;
 		return;
 	}
 	enum mp_type type3 = sql_value_type(arg3);
-	if (type3 != MP_NIL && type3 != MP_STR && !mp_type_is_bloblike(type3)) {
+	if (type3 != MP_NIL && type3 != MP_STR && type3 != MP_BIN) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 sql_value_to_diag_str(arg2), "string or varbinary");
 		context->is_aborted = true;