[tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Jan 22 18:41:32 MSK 2019
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 at 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);
> }
> }
More information about the Tarantool-patches
mailing list