[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 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).
> 
> Still do not understand. As I see, zAff was taken from exprINAffinity().
> This function allocates a string of affinities of length
> sqlite3ExprVectorSize(pExpr->pLeft). But nVector was initialized with
> exactly the same value. So strlen(zAff) should be equal to nVector, and
> your function sql_affinity_str_to_field_type_str() should return already
> correct array. I tried to test it and faced into a very strange situation.
> 
> Firstly, it appeared that already at the beginning of the
> function, after this code:
> 
>    zAff = exprINAffinity(pParse, pExpr);
>    nVector = sqlite3ExprVectorSize(pExpr->pLeft);
> 
> They are not already equal. This assertion fails:
> 
>    strlen(zAff) == nVector.
> 
> I tried to print their values in debug, and found, that
> zAff is stored in a buffer of length 2, but both bytes are
> zeros. Why? It is a first strange situation.

In a nutshell: owing to the fact that AFFINITY_UNDEFINED equals to 0.

Long story:

Consider simple example:

SELECT NULL IN (2, 3, 4, NULL)

When we reach sqlite3ExprCodeIN(), pExpr represents AST with the root in IN operator:

(lldb) print (int)pExpr->op
(int) $4 = 12

#define TK_IN                              12

LHS of the root is NULL (terminal symbol):

(lldb) print *pExpr->pLeft
(Expr) $6 = {
  op = 'K'
   = (affinity = AFFINITY_UNDEFINED, on_conflict_action = ON_CONFLICT_ACTION_NONE)
  flags = 8388612
  u = (zToken = "NULL", iValue = 59018336)

So nVector is 1. Moreover, NULL's affinity is UNDEFINED.

That is due to assigment to all expression by default
UNDEFINED affinity (as you may remember, I fixed it in
the next patch by setting it to SCALAR):

811  /* Construct a new Expr object from a single identifier.  Use the
812  ** new Expr to populate pOut.  Set the span of pOut to be the identifier
813  ** that created the expression.
814  */
815  static void spanExpr(ExprSpan *pOut, Parse *pParse, int op, Token t){
816    Expr *p = sqlite3DbMallocRawNN(pParse->db, sizeof(Expr)+t.n+1);
817    if( p ){
818      memset(p, 0, sizeof(Expr));
819      switch (op) {
820      case TK_STRING:
821        p->affinity = AFFINITY_TEXT;
822        break;
823      case TK_BLOB:
824        p->affinity = AFFINITY_BLOB;

Since NULL token is out of TK_STRING, TK_BLOB etc enumeration,
its affinity is 0, i.e. UNDERFINED.

Therefore, we see that nVector is 1, so zAff is of lenght 2
(because of additional null-termination), and finally,
sql_affinity_str_to_field_type_str() returns shorter array,
then is should be.

Fortunately, it doesn’t affect further patches, since there
we get rid of affinity, and SCALAR in enum is not 0.

> Secondly, your function sql_affinity_str_to_field_type_str()
> in such a case returns a buffer of one byte - [field_type_MAX],
> because it uses strlen(affinity_str) + 1, which == 1 here.
> 
> Further, you do types[nVector] = field_type_MAX, so you are
> out of array bounds, because nVector == 1, not 0. The only
> reason why it does not fail is that malloc returns more
> memory than necessary.
> 
> Please, check out if I am right, and fix the bug if so.
> 
> The assertion fails in sql-tap/randexpr1.test.lua.

As a workaround (this problem is automatically resolved
in next patch) I suggest to pass to sql_affinity_str_to..
len of affinity array:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 514d0ca9d..3adc6e262 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -521,17 +521,16 @@ sql_field_type_to_affinity(enum field_type field_type)
 }
 
 enum field_type *
-sql_affinity_str_to_field_type_str(const char *affinity_str)
+sql_affinity_str_to_field_type_str(const char *affinity_str, uint32_t len)
 {
        if (affinity_str == NULL)
                return NULL;
-       size_t len = strlen(affinity_str) + 1;
-       size_t sz = len * sizeof(enum field_type);
+       size_t sz = (len + 1) * sizeof(enum field_type);
        enum field_type *types =
                (enum field_type *) sqlite3DbMallocRaw(sql_get(), sz);
-       for (uint32_t i = 0; i < len - 1; ++i)
+       for (uint32_t i = 0; i < len; ++i)
                types[i] = sql_affinity_to_field_type(affinity_str[i]);
-       types[len - 1] = field_type_MAX;
+       types[len] = field_type_MAX;
        return types;
 }
 
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index d263c1bb5..b02089bc2 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3177,8 +3177,8 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */
         * of the RHS using the LHS as a probe.  If found, the result is
         * true.
         */
-       enum field_type *types = sql_affinity_str_to_field_type_str(zAff);
-       types[nVector] = field_type_MAX;
+       enum field_type *types = sql_affinity_str_to_field_type_str(zAff,
+                                                                   nVector);
        sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, (char *)types,
                          P4_DYNAMIC);
        if (destIfFalse == destIfNull) {
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 4b6983842..ef4514374 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1204,7 +1204,8 @@ selectInnerLoop(Parse * pParse,           /* The parser context */
                                       (unsigned int)nResultCol);
                                enum field_type *types =
                                        sql_affinity_str_to_field_type_str(
-                                               pDest->zAffSdst);
+                                               pDest->zAffSdst,
+                                               strlen(pDest->zAffSdst));
                                sqlite3VdbeAddOp4(v, OP_MakeRecord, regResult,
                                                  nResultCol, r1, (char *)types,
                                                  P4_DYNAMIC);
@@ -1631,7 +1632,8 @@ generateSortTail(Parse * pParse,  /* Parsing context */
                               sqlite3Strlen30(pDest->zAffSdst));
 
                        enum field_type *types =
-                               sql_affinity_str_to_field_type_str(pDest->zAffSdst);
+                               sql_affinity_str_to_field_type_str(pDest->zAffSdst,
+                                                                  strlen(pDest->zAffSdst));
                        sqlite3VdbeAddOp4(v, OP_MakeRecord, regRow, nColumn,
                                          regTupleid, (char *)types,
                                          P4_DYNAMIC);
@@ -3070,7 +3072,8 @@ generateOutputSubroutine(struct Parse *parse, struct Select *p,
                        testcase(in->nSdst > 1);
                        r1 = sqlite3GetTempReg(parse);
                        enum field_type *types =
-                               sql_affinity_str_to_field_type_str(dest->zAffSdst);
+                               sql_affinity_str_to_field_type_str(dest->zAffSdst,
+                                                                  strlen(dest->zAffSdst));
                        sqlite3VdbeAddOp4(v, OP_MakeRecord, in->iSdst,
                                          in->nSdst, r1, (char *)types,
                                          P4_DYNAMIC);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 729e5257c..79999509c 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3454,7 +3454,7 @@ enum affinity_type
 sql_field_type_to_affinity(enum field_type field_type);
 
 enum field_type *
-sql_affinity_str_to_field_type_str(const char *affinity_str);
+sql_affinity_str_to_field_type_str(const char *affinity_str, uint32_t len);
 
 /**
  * Compile view, i.e. create struct Select from
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index efbc91cf8..e3e7c5222 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -398,8 +398,7 @@ codeApplyAffinity(Parse * pParse, int base, int n, char *zAff)
 
        if (n > 0) {
                enum field_type *types =
-                       sql_affinity_str_to_field_type_str(zAff);
-               types[n] = field_type_MAX;
+                       sql_affinity_str_to_field_type_str(zAff, n);
                sqlite3VdbeAddOp4(v, OP_ApplyType, base, n, 0, (char *)types,
                                  P4_DYNAMIC);

> Also, see 2 comments below.
> 
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index 58e35e0d9..d263c1bb5 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -2854,8 +2854,17 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
>> 						jmpIfDynamic = -1;
>> 					}
>> 					r3 = sqlite3ExprCodeTarget(pParse, pE2, r1);
>> +					enum field_type type =
>> +						sql_affinity_to_field_type(affinity);
>> +					size_t sz = 2 * sizeof(enum field_type);
>> +					enum field_type *types =
>> +						sqlite3DbMallocRaw(pParse->db,
>> +								   sz);
>> +					types[0] = type;
>> +					types[1] = field_type_MAX;
> 
> 1. Please, either check for malloc result, or just use the
> same method as in other places - declare it on the stack and pass
> sizeof(this_array) instead of P4_DYNAMIC.

Sorry, I accidentally added it to diff - it was proposal to refactoring
(unsuccessful). Returned to its previous state.

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

>> 	 				sqlite3VdbeAddOp4(v, OP_MakeRecord, r3,
>> -							  1, r2, &affinity, 1);
>> +							  1, r2, (char *)types,
>> +							  P4_DYNAMIC);
>> 					sqlite3ExprCacheAffinityChange(pParse,
>> 								       r3, 1);
>> 					sqlite3VdbeAddOp2(v, OP_IdxInsert, r2,
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 24d992284..80d2fd0aa 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -2881,7 +2873,7 @@ case OP_MakeRecord: {
>> 	 * of the record to data0.
>> 	 */
>> 	nField = pOp->p1;
>> -	zAffinity = pOp->p4.z;
>> +	enum field_type *types = (enum field_type *)pOp->p4.z;
> 
> 2. Maybe, it is worth adding enum field_type *types into VdbeOp.p4union
> and do not cast. Like we did with many other pointers.

Thanks for suggestion, but I guess it is not necessary now.
Lets consider this refactoring a bit later.