[patches] [PATCH 2/3] sql: add type attribute to struct Column

[n.pettik]

In order to avoid send excess mails to patches mailing list,
I won’t send patch v.2 (since it contains few minor fixes).
So, just inline changes I have made:

>> In order to remove colFlag from struct Column, it is necessary to delete
>> usages of this field as an indicator of column with specified type, i.e.
>> having set COLFLAG_HASTYPE.  This patch adds to struct Column new field
> to struct Column new field -> new field to `struct Column`

As you wish. Rephrased.

>> +		/* TODO: convert string of type into runtime FIELD_TYPE value. */
>> +		if (sqlite3StrNICmp(pType->z, "INTEGER", 7) == 0 ||
>> +		    sqlite3StrNICmp(pType->z, "INT", 3) == 0) {
> this condition is equivalent to just
> `sqlite3StrNICmp(pType->z, "INT", 3) == 0) `
> Replace `sqlite3StrNICmp` with `sqlite3StrICmp`

In fact, you are wrong too, since pType->z contains full string
of ‘create table’ statement. However, pTyze->n represents length of type substring,
so to make it comparing correctly, it is enough to add length checking:

+		if ((sqlite3StrNICmp(pType->z, "INTEGER", 7) == 0 &&
+		     pType->n == 7) ||
+		    (sqlite3StrNICmp(pType->z, "INT", 3) == 0 &&
+		     pType->n == 3)) {

>> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
>> index 3eb3248c6..4aa3b2961 100644
>> --- a/src/box/sql/pragma.c
>> +++ b/src/box/sql/pragma.c
>> @@ -368,8 +368,8 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
>>  					       || pCol->pDflt->op == TK_SPAN);
>>  					sqlite3VdbeMultiLoad(v, 1, "issisi",
>>  							     i, pCol->zName,
>> -							     sqlite3ColumnType
>> -							     (pCol, ""),
>> +							     field_type_strs[sqlite3ColumnType
> too long line

Fixed.

> replace `if (pTab) {` with `if (!pTab)` above …

Why should I do this? I don't get it...

>> -char *
>> -sqlite3ColumnType(Column * pCol, char *zDflt)
>> +enum field_type
>> +sqlite3ColumnType(Column * pCol)
>>  {
>> -	if ((pCol->colFlags & COLFLAG_HASTYPE) == 0)
>> -		return zDflt;
>> -	return pCol->zName + strlen(pCol->zName) + 1;
>> +	return pCol->type;
>>  }
> Make this function inline or #define

Done. (However, I suppose compiler is smart enough to inline this function
without explicit directives)

>> +/* Return true if given column is part of primary key. */
>> +bool
>> +table_column_is_primkey(Table *table, uint32_t column)
> I do not like primkey word &&
> The name is a little confusing, because one can suppose that the
> function checks if column is a primary key, while it actually checks
> if pk contains the column.

As you wish. Renamed to ‘table_column_is_in_pk'.

>> +{
>> +	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
>> +	struct space *space = space_by_id(space_id);
>> +	assert(space != NULL);
>> +
>> +	struct index *primary_idx = index_find(space, 0 /* PK */);
>> +	/* Views don't have any indexes. */
>> +	if (primary_idx == NULL)
>> +		return false;
>> +	struct index_def *idx_def = primary_idx->def;
>> +	uint64_t pk_mask = idx_def->key_def->column_mask;
>> +	if (column < 63) {
> Are there any constants in Tarantool for this purpose? (63)

Unfortunately, aren’t. It is just (the number of bits in uint64_t) - 1 :)

> 
>> +		return pk_mask & (1UL << column);
>> +	} else if ((pk_mask & (1UL << 1)) != 0) {
> magic

Added extensive comment to this function explaining this logic:

+/*
+ * Return true if given column is part of primary key.
+ * If field number is less than 63, corresponding bit
+ * in column mask is tested. Otherwise, check whether 64-th bit
+ * in mask is set or not. If it is set, then iterate through
+ * key parts of primary index and check field number.
+ * In case it isn't set, there are no key columns among
+ * the rest of fields.
+ */

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180226/1aa53667/attachment.html>