[tarantool-patches] Re: [PATCH 5/5] sql: introduce VARBINARY column type

[Vladislav Shpilevoy]

Thanks for the patch! See 4 comments below.

On 24/07/2019 13:42, Nikita Pettik wrote:
> Current patch introduces new type available in SQL:
>  - VARBINARY now is reserved keyword;
>  - Allow to specify VARBINARY column and CAST type;
>  - All literals which start from 'x' are assumed to be of this type;
>  - There's no available implicit or explicit conversions between
>    VARBINARY and other types;
>  - Under the hood all values of VARBINARY type are stored as MP_BIN
>    msgpack format type.
>
> Closes #4206
> ---
>  extra/mkkeywordhash.c                      |   1 +
>  src/box/sql/expr.c                         |   2 +-
>  src/box/sql/func.c                         |   4 +-
>  src/box/sql/parse.y                        |   3 +-
>  src/box/sql/vdbe.c                         |  26 ++-
>  src/box/sql/vdbemem.c                      |   5 +
>  test/sql-tap/keyword1.test.lua             |   3 +-
>  test/sql/gh-3888-values-blob-assert.result |   4 +-
>  test/sql/iproto.result                     |   4 +-
>  test/sql/misc.result                       |   2 +-
>  test/sql/types.result                      | 256 ++++++++++++++++++++++++++++-
>  test/sql/types.test.lua                    |  60 +++++++
>  12 files changed, 345 insertions(+), 25 deletions(-)
>
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 826232f99..d0f0cb4f5 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2094,20 +2098,24 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
>  			}
>  			break;
>  		}
> -	} else if ((flags1 | flags3) & MEM_Bool) {
> +	} else if (((flags1 | flags3) & MEM_Bool) != 0 ||
> +		   ((flags1 | flags3) & MEM_Blob) != 0) {

1. Nit:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index d0f0cb4f5..ea7c9f25f 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2098,8 +2098,7 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 			}
 			break;
 		}
-	} else if (((flags1 | flags3) & MEM_Bool) != 0 ||
-		   ((flags1 | flags3) & MEM_Blob) != 0) {
+	} else if (((flags1 | flags3) & (MEM_Bool | MEM_Blob)) != 0) {
 		/*
 		 * If one of values is of type BOOLEAN or VARBINARY,
 		 * then the second one must be of the same type as



2. Please, add tests on varbinary values binding.

3. BLOB keyword is reserved, but also it is used in parse.y:980.
Should not it be deleted from all the rules, and be just
reserved?

4. Additionally (I bet, you knew I would ask), what are we going
to do with all mentionings of blob in the code? We have word 'blob'
mentioned 368 times in all the SQL sources, including comments,
parts of names.

In wherecode.c:644 we have a CREATE TABLE example, using BLOB -
this one definitely should be fixed. What about other places?
We could replace 'blob' -> 'bin','binary', 'vbin', ... . It
should not take much time.