[tarantool-patches] Re: [PATCH 7/8] sql: clean-up affinity from SQL source code

[Vladislav Shpilevoy]

Thanks for the patch! See 5 comments below.

On 28/12/2018 12:34, Nikita Pettik wrote:
> Replace remains of affinity usage in SQL parser, query optimizer and
> VDBE. Don't add affinity to field definition when table is encoded into
> msgpack.  Remove field type <-> affinity converters, since now we can
> operate directly on field type.
> 
> Part of #3698
> ---
>   src/box/sql.c           |  6 +-----
>   src/box/sql/build.c     | 52 ------------------------------------------------
>   src/box/sql/expr.c      | 11 +++++-----
>   src/box/sql/insert.c    | 39 ++++++------------------------------
>   src/box/sql/select.c    | 17 +++++-----------
>   src/box/sql/sqliteInt.h | 53 -------------------------------------------------
>   src/box/sql/vdbe.c      |  4 ++--
>   src/box/sql/where.c     | 10 ----------
>   src/box/sql/wherecode.c | 29 ++++++++++-----------------
>   src/box/sql/whereexpr.c |  3 ++-
>   test/sql/types.result   | 15 +++++++-------
>   test/sql/upgrade.result |  6 +++---
>   12 files changed, 42 insertions(+), 203 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 32606dac3..22b64b526 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -311,8 +311,7 @@ binaryCompareP5(Expr * pExpr1, Expr * pExpr2, int jumpIfNull)
>   {
>   	enum field_type lhs = sql_expr_type(pExpr2);
>   	enum field_type rhs = sql_expr_type(pExpr1);
> -	u8 type_mask = sql_field_type_to_affinity(sql_type_result(rhs, lhs)) |
> -		       (u8) jumpIfNull;
> +	u8 type_mask = sql_type_result(rhs, lhs) | (u8) jumpIfNull;

1. Are you sure that we can | jumpIfNull with enum field_type? Look at
this code:

	#define SQLITE_KEEPNULL     0x08	/* Used by vector == or <> */
	#define SQLITE_JUMPIFNULL   0x10	/* jumps if either operand is NULL */
	#define SQLITE_STOREP2      0x20	/* Store result in reg[P2] rather than jump */
	#define SQLITE_NULLEQ       0x80	/* NULL=NULL */
	#define SQLITE_NOTNULL      0x90	/* Assert that operands are never NULL */

SQLite states that these values can be safely ORed with type, but
SQLITE_KEEPNULL == FIELD_TYPE_MAP - mess.

>   	return type_mask;
>   }
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index cc3e2f2fd..f3008094b 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -3067,10 +3061,9 @@ generateOutputSubroutine(struct Parse *parse, struct Select *p,
>   			int r1;
>   			testcase(in->nSdst > 1);
>   			r1 = sqlite3GetTempReg(parse);
> -			const char *type_str =
> -				sql_affinity_str_to_field_type_str(dest->zAffSdst);
>   			sqlite3VdbeAddOp4(v, OP_MakeRecord, in->iSdst,
> -					  in->nSdst, r1, type_str, P4_DYNAMIC);
> +					  in->nSdst, r1, dest->zAffSdst,
> +					  in->nSdst);

2. As we learned from the previous patch, p4 here is not
a length. It is a type of memory - static or dynamic.

>   			sqlite3ExprCacheAffinityChange(parse, in->iSdst,
>   						       in->nSdst);
>   			sqlite3VdbeAddOp2(v, OP_IdxInsert, r1, dest->reg_eph);
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index ac5390f92..88c45aaa3 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -701,7 +701,6 @@ termCanDriveIndex(WhereTerm * pTerm,	/* WHERE clause term to check */
>   		return 0;
>   	if (pTerm->u.leftColumn < 0)
>   		return 0;
> -	aff = pSrc->pTab->aCol[pTerm->u.leftColumn].affinity;
>   	if (!sqlite3IndexAffinityOk(pTerm->pExpr, aff))

3. This function does not exist since the previous commit. Looks
like this code is not even compiled.

>   		return 0;
>   	return 1;

4. AFFINITY_MASK still exists.

5. wherecode.c and vdbeaux.c still use AFFINITY_BLOB in comments.