[tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime

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

Thanks for the patch! See 8 comments below.

> sql: replace field type with affinity for VDBE runtime

1. Maybe vice versa? Replace affinity with field type?

On 28/12/2018 12:34, Nikita Pettik wrote:
> This stage of affinity removal requires introducing of auxiliary
> intermediate function to convert array of affinity values to field type
> values. The rest of job done in this commit is a straightforward
> refactoring.
> 
> Part of #3698
> ---
>   src/box/sql/analyze.c                |  5 +-
>   src/box/sql/build.c                  | 13 +++++
>   src/box/sql/delete.c                 | 16 +++----
>   src/box/sql/expr.c                   | 21 ++++----
>   src/box/sql/fkey.c                   |  3 +-
>   src/box/sql/insert.c                 | 31 ++++++++----
>   src/box/sql/select.c                 | 17 +++++--
>   src/box/sql/sqliteInt.h              | 13 +++--
>   src/box/sql/update.c                 | 17 ++++---
>   src/box/sql/vdbe.c                   | 92 ++++++++++++++++--------------------
>   src/box/sql/vdbeInt.h                |  2 +-
>   src/box/sql/vdbemem.c                | 18 +++----
>   src/box/sql/where.c                  |  2 +-
>   src/box/sql/wherecode.c              |  6 +--
>   test/sql-tap/cast.test.lua           | 10 ++--
>   test/sql-tap/tkt-80e031a00f.test.lua |  8 ++--
>   16 files changed, 156 insertions(+), 118 deletions(-)
> 
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 51c63fa7a..a04dc8681 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -993,9 +993,10 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
>   		sqlite3VdbeAddOp2(v, OP_Next, idx_cursor, next_row_addr);
>   		/* Add the entry to the stat1 table. */
>   		callStatGet(v, stat4_reg, STAT_GET_STAT1, stat1_reg);
> -		assert("BBB"[0] == AFFINITY_TEXT);
> +		char types[3] = { FIELD_TYPE_STRING, FIELD_TYPE_STRING,
> +				  FIELD_TYPE_STRING };

2. How about explicit type? Not char[], but enum field_type[] ? It will
look much more readable and convenient, IMO. Here and in other places.

'type_str', used now in all places instead of affinity str, looks crutchy.

>   		sqlite3VdbeAddOp4(v, OP_MakeRecord, tab_name_reg, 3, tmp_reg,
> -				  "BBB", 0);
> +				  types, 3);
>   		sqlite3VdbeAddOp4(v, OP_IdxInsert, tmp_reg, 0, 0,
>   				  (char *)stat1, P4_SPACEPTR);
>   		/* Add the entries to the stat4 table. */
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index e51e2db2a..b3f98c317 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -520,6 +520,19 @@ sql_field_type_to_affinity(enum field_type field_type)
>   	}
>   }
>   
> +char *

2. So not char *, but enum field_type *. Ok?

> +sql_affinity_str_to_field_type_str(const char *affinity_str)
> +{
> +	if (affinity_str == NULL)
> +		return NULL;
> +	size_t len = strlen(affinity_str) + 1;
> +	char *type_str = (char *) sqlite3DbMallocRaw(sql_get(), len);
> +	for (uint32_t i = 0; i < len - 1; ++i)
> +		type_str[i] = sql_affinity_to_field_type(affinity_str[i]);
> +	type_str[len - 1] = '\0';

2. Instead of 0 you can use field_type_MAX terminator, if we will move
to enum field_type[].

> +	return type_str;
> +}
> +
>   /*
>    * Add a new column to the table currently being constructed.
>    *
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index f9c42fdec..7b0d6b2fd 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -332,12 +332,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>   			 */
>   			key_len = 0;
>   			struct index *pk = space_index(space, 0);
> -			const char *zAff = is_view ? NULL :
> -					   sql_space_index_affinity_str(parse->db,
> -									space->def,
> -									pk->def);
> +			char *type_str = is_view ? NULL :
> +					 sql_index_type_str(parse->db,
> +							    pk->def);
>   			sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
> -					  reg_key, zAff, pk_len);
> +					  reg_key, type_str, is_view ? 0 :
> +							     P4_DYNAMIC);

3. How did it work before your patch? Looks like it was a leak. Before the
patch, pk_len was passed instead of STATIC/DYNAMIC.

>   			/* Set flag to save memory allocating one
>   			 * by malloc.
>   			 */
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 917e6e30b..c823c5a06 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -2858,8 +2858,11 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
>   						jmpIfDynamic = -1;
>   					}
>   					r3 = sqlite3ExprCodeTarget(pParse, pE2, r1);
> +					char type =
> +						sql_affinity_to_field_type(affinity);
>   	 				sqlite3VdbeAddOp4(v, OP_MakeRecord, r3,
> -							  1, r2, &affinity, 1);
> +							  1, r2, &type,
> +							  1);

4. I do not understand. Is p4type of sqlite3VdbeAddOp4 of OP_MakeRecord
a length of an affinity string, or a type of its allocation? Or both?

Also, I wonder how does this code works:

	if (zAffinity) {
		pRec = pData0;
		do{
			mem_apply_type(pRec++, *(zAffinity++));
		}while( zAffinity[0]);
	}

in OP_MakeRecord. It assumes, that zAffinity is a null-terminated string,
but in the code above you pass one char, without zero-termination.

>   					sqlite3ExprCacheAffinityChange(pParse,
>   								       r3, 1);
>   					sqlite3VdbeAddOp2(v, OP_IdxInsert, r2,
> @@ -3172,7 +3175,8 @@ sqlite3ExprCodeIN(Parse * pParse,	/* Parsing and code generating context */
>   	 * of the RHS using the LHS as a probe.  If found, the result is
>   	 * true.
>   	 */
> -	sqlite3VdbeAddOp4(v, OP_Affinity, rLhs, nVector, 0, zAff, nVector);
> +	char *type_str = sql_affinity_str_to_field_type_str(zAff);
> +	sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, type_str, nVector);

5. type_str is dynamically allocated, nVector (== p4type) is > 0. But freeP4
function in vdbeaux.c does not know how to free p4type > 0. So it is definitely
a leak. Please, validate all places where dynamic type string is created.

>   	if (destIfFalse == destIfNull) {
>   		/* Combine Step 3 and Step 5 into a single opcode */
>   		sqlite3VdbeAddOp4Int(v, OP_NotFound, pExpr->iTable,
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 4345af24e..369fb4b79 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -306,32 +306,35 @@ applyNumericAffinity(Mem *pRec, int bTryForInt)
>   }
>   
>   /**
> - * Processing is determine by the affinity parameter:
> + * Processing is determine by the field type parameter:
>    *
> - * AFFINITY_INTEGER:
> - * AFFINITY_REAL:
> - *    Try to convert mem to an integer representation or a
> - *    floating-point representation if an integer representation
> - *    is not possible.  Note that the integer representation is
> - *    always preferred, even if the affinity is REAL, because
> - *    an integer representation is more space efficient on disk.
> + * INTEGER:
> + *    If memory holds floating point value and it can be
> + *    converted without loss (2.0 - > 2), it's type is
> + *    changed to INT. Otherwise, simply return success status.
>    *
> - * AFFINITY_TEXT:
> - *    Convert mem to a text representation.
> + * NUMBER:
> + *    If memory holds INT or floating point value,
> + *    no actions take place.
>    *
> - * AFFINITY_BLOB:
> - *    No-op. mem is unchanged.
> + * STRING:
> + *    Convert mem to a string representation.
>    *
> - * @param record The value to apply affinity to.
> - * @param affinity The affinity to be applied.
> + * SCALAR:
> + *    Mem is unchanged, but flat is set to BLOB.

6. flat -> flag?

> + *
> + * @param record The value to apply type to.
> + * @param type_t The type to be applied.
>    */
>   static int
> -mem_apply_affinity(struct Mem *record, enum affinity_type affinity)
> +mem_apply_type(struct Mem *record, enum field_type f_type)
>   {
>   	if ((record->flags & MEM_Null) != 0)
>   		return 0;
> -	switch (affinity) {
> -	case AFFINITY_INTEGER:
> +	assert(f_type  < field_type_MAX);

7. Double white-space.

> +	switch (f_type) {
> +	case FIELD_TYPE_INTEGER:
> +	case FIELD_TYPE_UNSIGNED:
>   		if ((record->flags & MEM_Int) == MEM_Int)
>   			return 0;
>   		if ((record->flags & MEM_Real) == MEM_Real) {
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index 571b5af78..539296079 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -1200,7 +1200,7 @@ whereRangeSkipScanEst(Parse * pParse,		/* Parsing & code generating context */
>   	int nLower = -1;
>   	int nUpper = index->def->opts.stat->sample_count + 1;
>   	int rc = SQLITE_OK;
> -	u8 aff = sql_space_index_part_affinity(space->def, p, nEq);
> +	u8 aff = p->key_def->parts[nEq].type;

8. Why? Below in this function aff is used as affinity, not type.

>   
>   	sqlite3_value *p1 = 0;	/* Value extracted from pLower */
>   	sqlite3_value *p2 = 0;	/* Value extracted from pUpper */