[tarantool-patches] Re: [PATCH 3/9] sql: use msgpack types instead of custom ones

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 4 comments below.

On 14/04/2019 18:04, Nikita Pettik wrote:
> This patch provides straightforward refactoring replacing enum sql_type
> with enum mp_type. Note that we use msgpack format instead of field_type
> since it is more suitable when dealing with NULLs.
> ---
>  src/box/bind.c          |  37 ++++++------------
>  src/box/execute.c       |  12 +++---
>  src/box/lua/execute.c   |   7 +---
>  src/box/lua/lua_sql.c   |  12 +++---
>  src/box/sql/date.c      |   2 +-
>  src/box/sql/func.c      | 100 ++++++++++++++++++++++++------------------------
>  src/box/sql/legacy.c    |   2 +-
>  src/box/sql/sqlInt.h    |  16 ++------
>  src/box/sql/vdbeapi.c   |  66 ++++++++++----------------------
>  src/box/sql/whereexpr.c |   2 +-
>  10 files changed, 102 insertions(+), 154 deletions(-)
> 
> diff --git a/src/box/bind.c b/src/box/bind.c
> index 4b57e23c8..5aa79751a 100644
> --- a/src/box/bind.c
> +++ b/src/box/bind.c
> @@ -138,6 +121,7 @@ sql_bind_decode(struct sql_bind *bind, int i, const char **packet)
>  	default:
>  		unreachable();
>  	}
> +	bind->type = type;

1. struct sql_bind.type still is uint8_t and commented as
'SQL type of the value.'. Please, change its type to enum mp_type
and say in the comment 'MessagePack type of the value.'

>  	return 0;
>  }
>  > diff --git a/src/box/sql/date.c b/src/box/sql/date.c
> index 5f5272ea3..ead0c01d0 100644
> --- a/src/box/sql/date.c
> +++ b/src/box/sql/date.c
> @@ -937,7 +937,7 @@ isDate(sql_context * context, int argc, sql_value ** argv, DateTime * p)
>  	if (argc == 0) {
>  		return setDateTimeToCurrent(context, p);
>  	}
> -	if ((eType = sql_value_type(argv[0])) == SQL_FLOAT
> +	if ((eType = sql_value_type(argv[0])) == MP_DOUBLE

2. Firstly, the code is commented out. Secondly, sql_value_type
does not exist anymore. Thirdly, eType is of type int, not
enum mp_type, and can not be compared with MP_DOUBLE.

Please, rename sql_value_mp_type to sql_value_type, because we
do not have 'not mp' type anymore. Then this hunk will be ok
except the third objection.

>  	    || eType == SQL_INTEGER) {
>  		setRawDateNumber(p, sql_value_double(argv[0]));
>  	} else {
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 9adfeec67..3cdb119c8 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -87,10 +87,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
>  	pColl = sqlGetFuncCollSeq(context);
>  	assert(mask == -1 || mask == 0);
>  	iBest = 0;
> -	if (sql_value_type(argv[0]) == SQL_NULL)
> +	if (sql_value_mp_type(argv[0]) == MP_NIL)

3. Please, move that check into a separate static inline
function in a header file. It appeared to be used often.
A possible name 'sql_value_is_null'.

>  		return;
>  	for (i = 1; i < argc; i++) {
> -		if (sql_value_type(argv[i]) == SQL_NULL)
> +		if (sql_value_mp_type(argv[i]) == MP_NIL)
>  			return;
>  		if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >=
>  		    0) {
> @@ -139,15 +139,15 @@ lengthFunc(sql_context * context, int argc, sql_value ** argv)
>  
>  	assert(argc == 1);
>  	UNUSED_PARAMETER(argc);
> -	switch (sql_value_type(argv[0])) {
> -	case SQL_BLOB:
> -	case SQL_INTEGER:
> -	case SQL_FLOAT:{
> +	switch (sql_value_mp_type(argv[0])) {
> +	case MP_BIN:
> +	case MP_INT:
> +	case MP_DOUBLE:{

4. Probably you could fix part of this:
https://github.com/tarantool/tarantool/issues/4159 in scope of
this commit, alongside.

>  			sql_result_int(context,
>  					   sql_value_bytes(argv[0]));
>  			break;
>  		}
> -	case SQL_TEXT:{
> +	case MP_STR:{
>  			const unsigned char *z = sql_value_text(argv[0]);
>  			if (z == 0)