[tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Dec 29 20:42:28 MSK 2018
Thanks for the patch! See 8 comments below.
> sql: replace field type with affinity for VDBE runtime
1. Maybe vice versa? Replace affinity with field type?
On 28/12/2018 12:34, Nikita Pettik wrote:
> 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
> ---
> src/box/sql/analyze.c | 5 +-
> src/box/sql/build.c | 13 +++++
> src/box/sql/delete.c | 16 +++----
> src/box/sql/expr.c | 21 ++++----
> src/box/sql/fkey.c | 3 +-
> src/box/sql/insert.c | 31 ++++++++----
> src/box/sql/select.c | 17 +++++--
> src/box/sql/sqliteInt.h | 13 +++--
> src/box/sql/update.c | 17 ++++---
> src/box/sql/vdbe.c | 92 ++++++++++++++++--------------------
> src/box/sql/vdbeInt.h | 2 +-
> src/box/sql/vdbemem.c | 18 +++----
> src/box/sql/where.c | 2 +-
> src/box/sql/wherecode.c | 6 +--
> test/sql-tap/cast.test.lua | 10 ++--
> test/sql-tap/tkt-80e031a00f.test.lua | 8 ++--
> 16 files changed, 156 insertions(+), 118 deletions(-)
>
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 51c63fa7a..a04dc8681 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -993,9 +993,10 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
> sqlite3VdbeAddOp2(v, OP_Next, idx_cursor, next_row_addr);
> /* Add the entry to the stat1 table. */
> callStatGet(v, stat4_reg, STAT_GET_STAT1, stat1_reg);
> - assert("BBB"[0] == AFFINITY_TEXT);
> + char types[3] = { FIELD_TYPE_STRING, FIELD_TYPE_STRING,
> + FIELD_TYPE_STRING };
2. How about explicit type? Not char[], but enum field_type[] ? It will
look much more readable and convenient, IMO. Here and in other places.
'type_str', used now in all places instead of affinity str, looks crutchy.
> sqlite3VdbeAddOp4(v, OP_MakeRecord, tab_name_reg, 3, tmp_reg,
> - "BBB", 0);
> + types, 3);
> sqlite3VdbeAddOp4(v, OP_IdxInsert, tmp_reg, 0, 0,
> (char *)stat1, P4_SPACEPTR);
> /* Add the entries to the stat4 table. */
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index e51e2db2a..b3f98c317 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -520,6 +520,19 @@ sql_field_type_to_affinity(enum field_type field_type)
> }
> }
>
> +char *
2. So not char *, but enum field_type *. Ok?
> +sql_affinity_str_to_field_type_str(const char *affinity_str)
> +{
> + if (affinity_str == NULL)
> + return NULL;
> + size_t len = strlen(affinity_str) + 1;
> + char *type_str = (char *) sqlite3DbMallocRaw(sql_get(), len);
> + for (uint32_t i = 0; i < len - 1; ++i)
> + type_str[i] = sql_affinity_to_field_type(affinity_str[i]);
> + type_str[len - 1] = '\0';
2. Instead of 0 you can use field_type_MAX terminator, if we will move
to enum field_type[].
> + return type_str;
> +}
> +
> /*
> * Add a new column to the table currently being constructed.
> *
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index f9c42fdec..7b0d6b2fd 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -332,12 +332,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> */
> key_len = 0;
> struct index *pk = space_index(space, 0);
> - const char *zAff = is_view ? NULL :
> - sql_space_index_affinity_str(parse->db,
> - space->def,
> - pk->def);
> + char *type_str = is_view ? NULL :
> + sql_index_type_str(parse->db,
> + pk->def);
> sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
> - reg_key, zAff, pk_len);
> + reg_key, type_str, is_view ? 0 :
> + P4_DYNAMIC);
3. How did it work before your patch? Looks like it was a leak. Before the
patch, pk_len was passed instead of STATIC/DYNAMIC.
> /* 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?
Also, I wonder how does this code works:
if (zAffinity) {
pRec = pData0;
do{
mem_apply_type(pRec++, *(zAffinity++));
}while( zAffinity[0]);
}
in OP_MakeRecord. It assumes, that zAffinity is a null-terminated string,
but in the code above you pass one char, without zero-termination.
> sqlite3ExprCacheAffinityChange(pParse,
> r3, 1);
> sqlite3VdbeAddOp2(v, OP_IdxInsert, r2,
> @@ -3172,7 +3175,8 @@ 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);
> + char *type_str = sql_affinity_str_to_field_type_str(zAff);
> + sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, type_str, nVector);
5. type_str is dynamically allocated, nVector (== p4type) is > 0. But freeP4
function in vdbeaux.c does not know how to free p4type > 0. So it is definitely
a leak. Please, validate all places where dynamic type string is created.
> 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/vdbe.c b/src/box/sql/vdbe.c
> index 4345af24e..369fb4b79 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:
> *
> - * 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 flat is set to BLOB.
6. flat -> flag?
> + *
> + * @param record The value to apply type to.
> + * @param type_t The type to be applied.
> */
> static int
> -mem_apply_affinity(struct Mem *record, enum affinity_type affinity)
> +mem_apply_type(struct Mem *record, enum field_type f_type)
> {
> if ((record->flags & MEM_Null) != 0)
> return 0;
> - switch (affinity) {
> - case AFFINITY_INTEGER:
> + assert(f_type < field_type_MAX);
7. Double white-space.
> + 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.
>
> sqlite3_value *p1 = 0; /* Value extracted from pLower */
> sqlite3_value *p2 = 0; /* Value extracted from pUpper */
More information about the Tarantool-patches
mailing list