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

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

Thanks for the fixes!

>>
>>>   			/* 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?
> 
> Both. p4type > 0 means that p4type bytes are copied to freshly allocated memory
> and P4 set to DYNAMIC. sqlite3DbStrNDup() in vdbeChangeP4Full() reserves
> one more byte for NULL termination.

I see now, that OP_MakeRecord uses only p4.z and does not touch p4type, so it makes
no sense to pass here length of the field_type array. Please, use only P4_STATIC and
P4_DYNAMIC. At this moment this opcode is mess: somewhere 0 is passed, somewhere
P4_DYNAMIC, in other places length of a field_type array.

>>> +	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.
> 
> Am I missing smth?
> sqlite3Stat4ValueFromExpr -> stat4ValueFromExpr -> sqlite3ValueApplyAffinity -> mem_apply_type

Naming confuses. All this functions except mem_apply_type take "u8 affinity",
not "enum field_type type". Please, rename parameters and functions.

> Author: Nikita Pettik <korablev@tarantool.org>
> Date:   Fri Dec 21 13:23:07 2018 +0200
> 
>      sql: replace affinity with field type for VDBE runtime
>      
>      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
> 

See below 6 comments.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index e51e2db2a..514d0ca9d 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index f9c42fdec..ca6c49373 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -592,13 +592,13 @@ sql_generate_index_key(struct Parse *parse, struct index *index, int cursor,
>                   * is an integer, then it might be stored in the
>                   * table as an integer (using a compact
>                   * representation) then converted to REAL by an
> -                * OP_RealAffinity opcode. But we are getting
> +                * OP_Realify opcode. But we are getting

1. OP_RealAffinity still is mentioned in sqliteInt.h in
a comment.

>                   * ready to store this value back into an index,
>                   * where it should be converted by to INTEGER
> -                * again.  So omit the OP_RealAffinity opcode if
> +                * again.  So omit the OP_Realify opcode if
>                   * it is present
>                   */
> -               sqlite3VdbeDeletePriorOpcode(v, OP_RealAffinity);
> +               sqlite3VdbeDeletePriorOpcode(v, OP_Realify);
>          }
>          if (reg_out != 0)
>                  sqlite3VdbeAddOp3(v, OP_MakeRecord, reg_base, col_cnt, reg_out);
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 0a80ca622..ca39faf51 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -3172,7 +3177,10 @@ 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);
> +       enum field_type *types = sql_affinity_str_to_field_type_str(zAff);
> +       types[nVector] = field_type_MAX;
> +       sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, (char *)types,
> +                         P4_DYNAMIC);

2. Why do you need types[nVector] = field_type_MAX? As I see, before your patch
there was no additional zero termination.

>          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/update.c b/src/box/sql/update.c
> index 0e2d0fde8..fd74817ea 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -274,11 +274,12 @@ sqlite3Update(Parse * pParse,             /* The parser context */
>                  nKey = pk_part_count;
>                  regKey = iPk;
>          } else {
> -               const char *zAff = is_view ? 0 :
> -                                  sql_space_index_affinity_str(pParse->db, def,
> -                                                               pPk->def);
> +               enum field_type *types = is_view ? NULL :
> +                                        sql_index_type_str(pParse->db,
> +                                                           pPk->def);
>                  sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count,
> -                                 regKey, zAff, pk_part_count);
> +                                 regKey, (char *) types,
> +                                 is_view ? 0 : P4_DYNAMIC);

3. I guess, here and in other places, where type str is either 0
or not 0, you can always set P4_DYNAMIC, it looks a bit simpler
and works as well. "is_view ? 0 : P4_DYNAMIC" -> "P4_DYNAMIC".

>                  /*
>                   * Set flag to save memory allocating one by
>                   * malloc.
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 4345af24e..61d73b676 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:

4. Not sure, if determine is an adjective or a noun. It is
always a verb. So it should be 'is determine*D*', shouldn't it?

>    *
> - * 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 flag is set to BLOB.
> + *
> + * @param record The value to apply type to.
> + * @param type_t The type to be applied.

5. type_t -> f_type. By the way, why f_type and not just type?

>    */
>   static int
> -mem_apply_affinity(struct Mem *record, enum affinity_type affinity)
> +mem_apply_type(struct Mem *record, enum field_type f_type)
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index b124a1d53..efbc91cf8 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -396,9 +396,12 @@ codeApplyAffinity(Parse * pParse, int base, int n, char *zAff)
>                  n--;
>          }
>   
> -       /* Code the OP_Affinity opcode if there is anything left to do. */
>          if (n > 0) {
> -               sqlite3VdbeAddOp4(v, OP_Affinity, base, n, 0, zAff, n);
> +               enum field_type *types =
> +                       sql_affinity_str_to_field_type_str(zAff);
> +               types[n] = field_type_MAX;

6. The same as 2.

> +               sqlite3VdbeAddOp4(v, OP_ApplyType, base, n, 0, (char *)types,
> +                                 P4_DYNAMIC);
>                  sqlite3ExprCacheAffinityChange(pParse, base, n);
>          }
>   }