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

["n.pettik" <korablev@tarantool.org>]

>>>> 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.

Well, actually we can’t pass here neither of these args and that’s why:
STATIC and DYNAMIC indicate that memory which is passed through
P4 argument will be copied to new chunk of memory:

   985	void
   986	sqlite3VdbeChangeP4(Vdbe * p, int addr, const char *zP4, int n)
   987	{
   
…

  1008		if (n == P4_INT32) {
  1009			/* Note: this cast is safe, because the origin data point was an int
  1010			 * that was cast to a (const char *).
  1011			 */
  1012			pOp->p4.i = SQLITE_PTR_TO_INT(zP4);
  1013			pOp->p4type = P4_INT32;
  1014		} if (n == P4_BOOL) {
  1015			pOp->p4.b = *(bool*)zP4;
  1016			pOp->p4type = P4_BOOL;
  1017		} else {
  1018			assert(n < 0);
  1019			pOp->p4.p = (void *)zP4;
  1020			pOp->p4type = (signed char)n;
  1021		}
  1022	}

Both P4_STATIC and P4_DYNAMIC are less than 0, so pOp->p4.p points
to object on allocated on stack (in this particular case).

Passing n > 0 we are implying that n bytes of memory starting from zP4
must be copied to new chunk, which comes with P4_DYNAMIC

If you still have doubts, I assure you that I’ve already tried to replace it and
always get assertion faults like:
Assertion failed: (f_type < field_type_MAX), function mem_apply_type, file tarantool/src/box/sql/vdbe.c, line 334.

In other places (where we use P4_DYNAMIC), field_type array is allocated
on heap, so it can be freed after VDBE execution.

As an option we can allocate on heap this string before calling
sqlite3VdbeChangeP4, like this:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index ca39faf51..27bbc1b1c 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2860,11 +2860,15 @@ sqlite3CodeSubselect(Parse * pParse,    /* Parsing context */
                                        r3 = sqlite3ExprCodeTarget(pParse, pE2, r1);
                                        enum field_type type =
                                                sql_affinity_to_field_type(affinity);
-                                       enum field_type types[2] =
-                                               { type, field_type_MAX };
+                                       size_t sz = 2 * sizeof(enum field_type);
+                                       enum field_type *types =
+                                               sqlite3DbMallocRaw(pParse->db,
+                                                                  sz);
+                                       types[0] = type;
+                                       types[1] = field_type_MAX;
                                        sqlite3VdbeAddOp4(v, OP_MakeRecord, r3,
                                                          1, r2, (char *)types,
-                                                         sizeof(types));
+                                                         P4_DYNAMIC);


>>>> 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.

As you wish. I just didn’t want to mix functional
changes and non-functional refactoring since it
complicates review process.

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 579f68fed..462cd233f 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4375,8 +4375,9 @@ void sqlite3ValueSetStr(sqlite3_value *, int, const void *,
 void sqlite3ValueSetNull(sqlite3_value *);
 void sqlite3ValueFree(sqlite3_value *);
 sqlite3_value *sqlite3ValueNew(sqlite3 *);
-int sqlite3ValueFromExpr(sqlite3 *, Expr *, u8, sqlite3_value **);
-void sqlite3ValueApplyAffinity(sqlite3_value *, u8);
+int sqlite3ValueFromExpr(sqlite3 *, Expr *, enum field_type type,
+                        sqlite3_value **);
+void sqlite3ValueApplyAffinity(sqlite3_value *, enum field_type type);
 #ifndef SQLITE_AMALGAMATION
 extern const unsigned char sqlite3OpcodeProperty[];
 extern const char sqlite3StrBINARY[];
@@ -4600,7 +4601,8 @@ int sqlite3ExprCheckIN(Parse *, Expr *);
 void sqlite3AnalyzeFunctions(void);
 int sqlite3Stat4ProbeSetValue(Parse *, struct index_def *, UnpackedRecord **, Expr *, int,
                              int, int *);
-int sqlite3Stat4ValueFromExpr(Parse *, Expr *, u8, sqlite3_value **);
+int sqlite3Stat4ValueFromExpr(Parse *, Expr *, enum field_type type,
+                             sqlite3_value **);
 void sqlite3Stat4ProbeFree(UnpackedRecord *);
 
 /**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 61d73b676..32be5b9fa 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -394,9 +394,9 @@ int sqlite3_value_numeric_type(sqlite3_value *pVal) {
 void
 sqlite3ValueApplyAffinity(
        sqlite3_value *pVal,
-       u8 affinity)
+       enum field_type type)
 {
-       mem_apply_type((Mem *) pVal, affinity);
+       mem_apply_type((Mem *) pVal, type);
 }
 
 /*
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index cd71641b0..7dc5dbfa9 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -1161,7 +1161,7 @@ valueNew(sqlite3 * db, struct ValueNewStat4Ctx *p)
 static int
 valueFromFunction(sqlite3 * db,        /* The database connection */
                  Expr * p,     /* The expression to evaluate */
-                 u8 aff,       /* Affinity to use */
+                 enum field_type type,
                  sqlite3_value ** ppVal,       /* Write the new value here */
                  struct ValueNewStat4Ctx *pCtx /* Second argument for valueNew() */
     )
@@ -1199,7 +1199,7 @@ valueFromFunction(sqlite3 * db,   /* The database connection */
                }
                for (i = 0; i < nVal; i++) {
                        rc = sqlite3ValueFromExpr(db, pList->a[i].pExpr,
-                                                 aff, &apVal[i]);
+                                                 type, &apVal[i]);
                        if (apVal[i] == 0 || rc != SQLITE_OK)
                                goto value_from_function_out;
                }
@@ -1220,7 +1220,7 @@ valueFromFunction(sqlite3 * db,   /* The database connection */
                rc = ctx.isError;
                sqlite3ErrorMsg(pCtx->pParse, "%s", sqlite3_value_text(pVal));
        } else {
-               sqlite3ValueApplyAffinity(pVal, aff);
+               sqlite3ValueApplyAffinity(pVal, type);
                assert(rc == SQLITE_OK);
        }
        pCtx->pParse->rc = rc;
@@ -1253,7 +1253,7 @@ valueFromFunction(sqlite3 * db,   /* The database connection */
 static int
 valueFromExpr(sqlite3 * db,    /* The database connection */
              Expr * pExpr,     /* The expression to evaluate */
-             u8 affinity,      /* Affinity to use */
+             enum field_type type,
              sqlite3_value ** ppVal,   /* Write the new value here */
              struct ValueNewStat4Ctx *pCtx     /* Second argument for valueNew() */
     )
@@ -1284,7 +1284,7 @@ valueFromExpr(sqlite3 * db,       /* The database connection */
                testcase(rc != SQLITE_OK);
                if (*ppVal) {
                        sqlite3VdbeMemCast(*ppVal, aff);
-                       sqlite3ValueApplyAffinity(*ppVal, affinity);
+                       sqlite3ValueApplyAffinity(*ppVal, type);
                }
                return rc;
        }
@@ -1315,18 +1315,18 @@ valueFromExpr(sqlite3 * db,     /* The database connection */
                                goto no_mem;
                        sqlite3ValueSetStr(pVal, -1, zVal, SQLITE_DYNAMIC);
                }
-               if ((op == TK_INTEGER || op == TK_FLOAT)
-                   && affinity == AFFINITY_BLOB) {
-                       sqlite3ValueApplyAffinity(pVal, AFFINITY_REAL);
+               if ((op == TK_INTEGER || op == TK_FLOAT) &&
+                   type == FIELD_TYPE_SCALAR) {
+                       sqlite3ValueApplyAffinity(pVal, FIELD_TYPE_NUMBER);
                } else {
-                       sqlite3ValueApplyAffinity(pVal, affinity);
+                       sqlite3ValueApplyAffinity(pVal, type);
                }
                if (pVal->flags & (MEM_Int | MEM_Real))
                        pVal->flags &= ~MEM_Str;
        } else if (op == TK_UMINUS) {
                /* This branch happens for multiple negative signs.  Ex: -(-5) */
                if (SQLITE_OK ==
-                   sqlite3ValueFromExpr(db, pExpr->pLeft, affinity, &pVal)
+                   sqlite3ValueFromExpr(db, pExpr->pLeft, type, &pVal)
                    && pVal != 0) {
                        if ((rc = sqlite3VdbeMemNumerify(pVal)) != SQLITE_OK)
                                return rc;
@@ -1338,7 +1338,7 @@ valueFromExpr(sqlite3 * db,       /* The database connection */
                        } else {
                                pVal->u.i = -pVal->u.i;
                        }
-                       sqlite3ValueApplyAffinity(pVal, affinity);
+                       sqlite3ValueApplyAffinity(pVal, type);
                }
        } else if (op == TK_NULL) {
                pVal = valueNew(db, pCtx);
@@ -1364,7 +1364,7 @@ valueFromExpr(sqlite3 * db,       /* The database connection */
 #endif
 
        else if (op == TK_FUNCTION && pCtx != 0) {
-               rc = valueFromFunction(db, pExpr, affinity, &pVal, pCtx);
+               rc = valueFromFunction(db, pExpr, type, &pVal, pCtx);
        }
 
        *ppVal = pVal;
@@ -1393,11 +1393,11 @@ valueFromExpr(sqlite3 * db,     /* The database connection */
 int
 sqlite3ValueFromExpr(sqlite3 * db,     /* The database connection */
                     Expr * pExpr,      /* The expression to evaluate */
-                    u8 affinity,       /* Affinity to use */
+                    enum field_type type,
                     sqlite3_value ** ppVal     /* Write the new value here */
     )
 {
-       return pExpr ? valueFromExpr(db, pExpr, affinity, ppVal, 0) : 0;
+       return pExpr ? valueFromExpr(db, pExpr, type, ppVal, 0) : 0;
 }
 
 /*
@@ -1471,7 +1471,7 @@ sqlite3AnalyzeFunctions(void)
 static int
 stat4ValueFromExpr(Parse * pParse,     /* Parse context */
                   Expr * pExpr,        /* The expression to extract a value from */
-                  u8 affinity, /* Affinity to use */
+                  enum field_type type,
                   struct ValueNewStat4Ctx *pAlloc,     /* How to allocate space.  Or NULL */
                   sqlite3_value ** ppVal       /* OUT: New value object (or NULL) */
     )
@@ -1501,14 +1501,13 @@ stat4ValueFromExpr(Parse * pParse,      /* Parse context */
                                rc = sqlite3VdbeMemCopy((Mem *) pVal,
                                                        &v->aVar[iBindVar - 1]);
                                if (rc == SQLITE_OK) {
-                                       sqlite3ValueApplyAffinity(pVal,
-                                                                 affinity);
+                                       sqlite3ValueApplyAffinity(pVal, type);
                                }
                                pVal->db = pParse->db;
                        }
                }
        } else {
-               rc = valueFromExpr(db, pExpr, affinity, &pVal, pAlloc);
+               rc = valueFromExpr(db, pExpr, type, &pVal, pAlloc);
        }
 
        assert(pVal == 0 || pVal->db == db);
@@ -1606,11 +1605,11 @@ sqlite3Stat4ProbeSetValue(Parse * pParse,       /* Parse context */
 int
 sqlite3Stat4ValueFromExpr(Parse * pParse,      /* Parse context */
                          Expr * pExpr, /* The expression to extract a value from */
-                         u8 affinity,  /* Affinity to use */
+                         enum field_type type,
                          sqlite3_value ** ppVal        /* OUT: New value object (or NULL) */
     )
 {
-       return stat4ValueFromExpr(pParse, pExpr, affinity, 0, ppVal);
+       return stat4ValueFromExpr(pParse, pExpr, type, 0, ppVal);

>> 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.

Removed:

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 579f68fed..cbdd1f1a3 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4459,7 +4460,7 @@ int sqlite3ResolveOrderGroupBy(Parse *, Select *, ExprList *, const char *);
  * function is capable of transforming these types of expressions into
  * sqlite3_value objects.
  *
- * If parameter iReg is not negative, code an OP_RealAffinity instruction
+ * If parameter iReg is not negative, code an OP_Realify instruction
  * on register iReg. This is used when an equivalent integer value is
  * stored in place of an 8-byte floating point value in order to save
  * space.


>> 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.

To cut off array of types. Before my patch, first nVector bytes
were copied inside vdbeChangeP4Full() and automatically
terminated with NULL (sqlite3DbStrNDup).

>> sqlite3VdbeAddOp4(v, OP_Affinity, rLhs, nVector, 0, zAff, nVector);


Now it makes no sense to copy this string: we can set
array termination and pass it with dynamic flag.

>> --- 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”.

Ok, indeed it is valid replacement:

diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index ca6c49373..a6e7547f5 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -336,8 +336,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
                                                 sql_index_type_str(parse->db,
                                                                    pk->def);
                        sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
-                                         reg_key, (char *)types, is_view ? 0 :
-                                                                 P4_DYNAMIC);
+                                         reg_key, (char *)types, P4_DYNAMIC);
                        /* Set flag to save memory allocating one
                         * by malloc.

diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index fd74817ea..cea840a99 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -278,8 +278,7 @@ sqlite3Update(Parse * pParse,               /* The parser context */
                                         sql_index_type_str(pParse->db,
                                                            pPk->def);
                sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count,
-                                 regKey, (char *) types,
-                                 is_view ? 0 : P4_DYNAMIC);
+                                 regKey, (char *) types, 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?

Yep, it is my (or not my) mistake. Fixed:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 61d73b676..a38075e2e 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -306,7 +306,7 @@ applyNumericAffinity(Mem *pRec, int bTryForInt)
 }
 
 /**
- * Processing is determine by the field type parameter:
+ * Processing is determined 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 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?

Hmm, I was sure that I got compile error since other
variable already occupied this name…
Nevermind, lets use simple ’type’ name:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 61d73b676..17921ce36 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -324,15 +324,15 @@ applyNumericAffinity(Mem *pRec, int bTryForInt)
  *    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.
+ * @param type The type to be applied.
  */
 static int
-mem_apply_type(struct Mem *record, enum field_type f_type)
+mem_apply_type(struct Mem *record, enum field_type type)
 {
        if ((record->flags & MEM_Null) != 0)
                return 0;
-       assert(f_type < field_type_MAX);
-       switch (f_type) {
+       assert(type < field_type_MAX);
+       switch (type) {
        case FIELD_TYPE_INTEGER:
        case FIELD_TYPE_UNSIGNED:
                if ((record->flags & MEM_Int) == MEM_Int)


>> 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.

See answer above: here we need only first n fields,
not the whole array.