[tarantool-patches] Re: [PATCH 05/10] sql: remove affinity string of columns from Index

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

>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
>> index 0be68830a..cb16a7c0d 100644
>> --- a/src/box/sql/delete.c
>> +++ b/src/box/sql/delete.c
>> @@ -352,7 +352,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>  			key_len = 0;
>>  			struct Index *pk = sqlite3PrimaryKeyIndex(table);
>>  			const char *zAff = is_view ? NULL :
>> -					  sqlite3IndexAffinityStr(parse->db, pk);
>> +					  sql_index_affinity_str(parse->db,
>> +								 pk->def);
> 
> 1. sql_index_affinity_str makes extra lookup in space cache. Here
> (in sql_table_delete_from) space ptr is known already.
> 
> Same in fkey_lookup_parent. Maybe it would be better to pass space ptr
> alongside with index_def? And name it sql_space_index_affinity_str.

Ok, time to pull up struct space:

+++ b/src/box/sql/delete.c
@@ -352,8 +352,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
                        key_len = 0;
                        struct Index *pk = sqlite3PrimaryKeyIndex(table);
                        const char *zAff = is_view ? NULL :
-                                         sql_index_affinity_str(parse->db,
-                                                                pk->def);
+                                          sql_space_index_affinity_str(parse->db,
+                                                                       space->def,
+                                                                       pk->def);
                        sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,

+++ b/src/box/sql/fkey.c
@@ -256,7 +256,8 @@ fkey_lookup_parent(struct Parse *parse_context, struct space *parent,
        struct index *idx = space_index(parent, referenced_idx);
        assert(idx != NULL);
        sqlite3VdbeAddOp4(v, OP_MakeRecord, temp_regs, field_count, rec_reg,
-                         sql_index_affinity_str(parse_context->db, idx->def),
+                         sql_space_index_affinity_str(parse_context->db,
+                                                      parent->def, idx->def),

+++ b/src/box/sql/insert.c
@@ -59,25 +59,24 @@ sqlite3OpenTable(Parse * pParse,    /* Generate code into this VDBE */
 }
 
 char *
-sql_index_affinity_str(struct sqlite3 *db, struct index_def *def)
+sql_space_index_affinity_str(struct sqlite3 *db, struct space_def *space_def,
+                            struct index_def *idx_def)
 {
-       uint32_t column_count = def->key_def->part_count;
+       uint32_t column_count = idx_def->key_def->part_count;
        char *aff = (char *)sqlite3DbMallocRaw(db, column_count + 1);
        if (aff == NULL)
                return NULL;
-       struct space *space = space_by_id(def->space_id);
-       assert(space != NULL);
        /*
         * Table may occasionally come from Lua, so lets
         * gentle process this case by setting default
         * affinity for it.
         */
-       if (space->def->fields == NULL) {
+       if (space_def->fields == NULL) {
                memset(aff, AFFINITY_BLOB, column_count);
        } else {
                for (uint32_t i = 0; i < column_count; i++) {
-                       uint32_t x = def->key_def->parts[i].fieldno;
-                       aff[i] = space->def->fields[x].affinity;
+                       uint32_t x = idx_def->key_def->parts[i].fieldno;
+                       aff[i] = space_def->fields[x].affinity;

+++ b/src/box/sql/sqliteInt.h
@@ -4297,11 +4297,14 @@ int sqlite3VarintLen(u64 v);
  * is allocated on heap.
  *
  * @param db Database handle.
- * @param def index_def where from affinity to be extracted.
+ * @param space_def Definition of space index belongs to.
+ * @param idx_def Definition of index from which affinity
+ *                to be extracted.
  * @retval Allocated affinity string, or NULL on OOM.
  */
 char *
-sql_index_affinity_str(struct sqlite3 *db, struct index_def *def);
+sql_space_index_affinity_str(struct sqlite3 *db, struct space_def *space_def,
+                            struct index_def *idx_def);

+++ b/src/box/sql/update.c
@@ -347,7 +347,8 @@ sqlite3Update(Parse * pParse,               /* The parser context */
                regKey = iPk;
        } else {
                const char *zAff = is_view ? 0 :
-                                  sql_index_affinity_str(pParse->db, pPk->def);
+                                  sql_space_index_affinity_str(pParse->db, def,
+                                                               pPk->def);

+++ b/src/box/sql/wherecode.c
@@ -724,11 +724,17 @@ codeAllEqualityTerms(Parse * pParse,      /* Parsing context */
 
        char *zAff;
        if (pIdx != NULL) {
+               struct space *space = space_by_id(pIdx->def->space_id);
+               assert(space != NULL);
                zAff = sqlite3DbStrDup(pParse->db,
-                                      sql_index_affinity_str(pParse->db,
-                                                             pIdx->def));
+                                      sql_space_index_affinity_str(pParse->db,
+                                                                   space->def,
+                                                                   pIdx->def));
        } else {
-               zAff = sql_index_affinity_str(pParse->db, idx_def);
+               struct space *space = space_by_id(idx_def->space_id);
+               assert(space != NULL);
+               zAff = sql_space_index_affinity_str(pParse->db, space->def,
+                                                   idx_def);


>>  			sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
>>  					  reg_key, zAff, pk_len);
>>  			/* Set flag to save memory allocating one
>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index ddd7f932b..2b6100ad5 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -112,15 +84,23 @@ sql_index_affinity_str(struct sqlite3 *db, struct index_def *def)
>>  		return NULL;
>>  	struct space *space = space_by_id(def->space_id);
>>  	assert(space != NULL);
>> -
>> +	/*
>> +	 * Table may occasionally come from Lua, so lets
>> +	 * gentle process this case by setting default
>> +	 * affinity for it.
>> +	 */
>> +	if (space->def->fields == NULL) {
>> +		memset(aff, AFFINITY_BLOB, column_count);
>> +		goto exit;
>> +	}
>>  	for (uint32_t i = 0; i < column_count; i++) {
>>  		uint32_t x = def->key_def->parts[i].fieldno;
>>  		aff[i] = space->def->fields[x].affinity;
>>  		if (aff[i] == AFFINITY_UNDEFINED)
>> -			aff[i] = 'A';
>> +			aff[i] = AFFINITY_BLOB;
>>  	}
>> +exit:
> 
> 2. Please, try to avoid extra labels when possible. They might
> help only when there are many error processing code snippets or
> when it allows to strongly reduce indentation. Otherwise it
> slightly confuses. I have fixed it on the branch.

Ok, got it. Thx for fix.

> 
>>  	aff[column_count] = '\0';
>> -
>>  	return aff;
>>  }
>>  diff --git a/src/box/sql/where.c b/src/box/sql/where.c
>> index db8d0bf80..794db0302 100644
>> --- a/src/box/sql/where.c
>> +++ b/src/box/sql/where.c
>> @@ -1224,7 +1220,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 = sqlite3IndexColumnAffinity(db, p, nEq);
>> +	u8 aff = sql_index_part_affinity(p->def, nEq);
> 
> 3. Same as 1 - extra lookup in the space cache. You have the space
> few lines above.

Fixed:

@@ -4611,13 +4614,15 @@ sql_stat4_column(struct sqlite3 *db, const char *record, uint32_t col_num,
 /**
  * Return the affinity for a single column of an index.
  *
+ * @param def Definition of space @idx belongs to.
  * @param idx Index to be investigated.
  * @param partno Affinity of this part to be returned.
  *
  * @retval Affinity of @partno index part.
  */
 enum affinity_type
-sql_index_part_affinity(struct index_def *idx, uint32_t partno);
+sql_space_index_part_affinity(struct space_def *def, struct index_def *idx,
+                             uint32_t partno);

+++ b/src/box/sql/where.c
@@ -1159,13 +1159,12 @@ whereRangeAdjust(WhereTerm * pTerm, LogEst nNew)
 }

+++ b/src/box/sql/vdbemem.c
@@ -39,6 +39,7 @@
 #include "sqliteInt.h"
 #include "vdbeInt.h"
 #include "tarantoolInt.h"
+#include "box/schema.h"
 
 #ifdef SQLITE_DEBUG
 /*
@@ -1548,11 +1549,14 @@ sqlite3Stat4ProbeSetValue(Parse * pParse,       /* Parse context */
                alloc.pIdx = pIdx;
                alloc.ppRec = ppRec;
 
+               struct space *space = space_by_id(pIdx->def->space_id);
+               assert(space != NULL);
                for (i = 0; i < nElem; i++) {
                        sqlite3_value *pVal = 0;
                        Expr *pElem =
                            (pExpr ? sqlite3VectorFieldSubexpr(pExpr, i) : 0);
-                       u8 aff = sql_index_part_affinity(pIdx->def, iVal + i);
+                       u8 aff = sql_space_index_part_affinity(space->def, pIdx->def,
+                                                              iVal + i);
 
 enum affinity_type
-sql_index_part_affinity(struct index_def *idx, uint32_t partno)
+sql_space_index_part_affinity(struct space_def *def, struct index_def *idx,
+                             uint32_t partno)
 {
        assert(partno < idx->key_def->part_count);
-       struct space *space = space_by_id(idx->space_id);
-       assert(space != NULL);
        uint32_t fieldno = idx->key_def->parts[partno].fieldno;
-       return space->def->fields[fieldno].affinity;
+       return def->fields[fieldno].affinity;
 } 

@@ -1220,7 +1219,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_index_part_affinity(p->def, nEq);
+       u8 aff = sql_space_index_part_affinity(space->def, p->def, nEq);