[tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime
n.pettik
korablev at tarantool.org
Mon Jan 28 19:39:50 MSK 2019
>>>> 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.
More information about the Tarantool-patches
mailing list