[tarantool-patches] Re: [PATCH v1 4/4] sql: get rid of Column structure

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 6 comments below.

On 16/07/2018 15:28, Kirill Shcherbatov wrote:
> Get rid of is_primkey in Column structure as it become
> redundant. Moved the last member coll with collation pointer
> to field_def structure. Finally, dropped Column.
> ---
>   src/box/field_def.c     |  1 +
>   src/box/field_def.h     |  2 ++
>   src/box/sql/alter.c     | 16 +++-------
>   src/box/sql/build.c     | 79 +++++++++++++++++--------------------------------
>   src/box/sql/resolve.c   | 11 +++----
>   src/box/sql/select.c    | 43 ++++++++++-----------------
>   src/box/sql/sqliteInt.h | 23 +++++++-------
>   7 files changed, 65 insertions(+), 110 deletions(-)
> 
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index fe54e55..4f50988 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -299,19 +299,11 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
>   	nAlloc = (((pNew->def->field_count - 1) / 8) * 8) + 8;
>   	assert((uint32_t)nAlloc >= pNew->def->field_count && nAlloc % 8 == 0 &&
>   	       nAlloc - pNew->def->field_count < 8);
> -	pNew->aCol =
> -	    (Column *) sqlite3DbMallocZero(db, sizeof(Column) * nAlloc);

1. And why do you need nAlloc now? Please, do not hurry. It is not ok that
you overlook such conspicuous things.

>   	/* FIXME: pNew->zName = sqlite3MPrintf(db, "sqlite_altertab_%s", pTab->zName); */
>   	/* FIXME: if (!pNew->aCol || !pNew->zName) { */
> -	if (!pNew->aCol) {
> -		assert(db->mallocFailed);
> -		goto exit_begin_add_column;
> -	}
> -	memcpy(pNew->aCol, pTab->aCol, sizeof(Column) * pNew->def->field_count);
>   	for (i = 0; i < pNew->def->field_count; i++) {
> -		Column *pCol = &pNew->aCol[i];
>   		/* FIXME: pNew->def->name = sqlite3DbStrDup(db, pCol->zName); */

2. What does this comment mean?

3. What about other fields? Please, use = field_def_default.

> -		pCol->coll = NULL;
> +		pNew->def->fields[i].coll = NULL;
>   		pNew->def->fields[i].coll_id = COLL_NONE;
>   	}
>   	pNew->pSchema = db->pSchema;
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index ec0bc4b..a6f3559 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c> @@ -131,10 +141,10 @@ check_on_conflict_replace_entries(struct Table *table)
>   	for (int i = 0; i < (int)table->def->field_count; i++) {
>   		enum on_conflict_action on_error =
>   			table->def->fields[i].nullable_action;
> +		struct Index *pk = sqlite3PrimaryKeyIndex(table);
>   		if (on_error == ON_CONFLICT_ACTION_REPLACE &&
> -		    table->aCol[i].is_primkey == false) {
> +		    !index_has_column(pk->aiColumn, pk->nColumn, i))

4. No. As I said you on the first review, you should not use here
scans or is_primkey. This check can be spread over other column check
functions, involved in the first commit with no additional scans.

>   			return true;
> -		}
>   	}
>   	/* Check all UNIQUE constraints. */
>   	for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) {
> @@ -915,21 +909,19 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
>   				goto primary_key_exit;
>   			}
>   			const char *zCName = pCExpr->u.zToken;
> -			for (iCol = 0;
> -				iCol < (int)pTab->def->field_count; iCol++) {
> -				if (strcmp
> -				    (zCName,
> -				     pTab->def->fields[iCol].name) == 0) {
> -					pCol = &pTab->aCol[iCol];
> -					pCol->is_primkey = 1;
> +			for (uint32_t collumn_id = 0;
> +			     collumn_id < pTab->def->field_count; collumn_id++) {
> +				if (strcmp(zCName,
> +					   pTab->def->fields[collumn_id].name)
> +				    == 0) {
> +					iCol = collumn_id;
>   					break;

5. Please, make the code more readable. You should avoid such long names
as 'collumn_id' (btw 'collumn' word does not exist), save pTab->def->fields
in a separate variable etc.

>   				}
>   			}
>   		}
>   	}
> -	assert(pCol == NULL || pCol == &pTab->aCol[iCol]);
> -	if (nTerm == 1 && pCol != NULL &&
> -	    (pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER) &&
> +	if (nTerm == 1 && iCol != -1 &&
> +	    pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER &&
>   	    sortOrder != SORT_ORDER_DESC) {
>   		assert(autoInc == 0 || autoInc == 1);
>   		pTab->iPKey = iCol;
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index ceb7e34..745ae2f 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1811,25 +1811,15 @@ sqlite3ColumnsFromExprList(Parse * parse, ExprList * expr_list, Table *table)
>   {
>   	/* Database connection */
>   	sqlite3 *db = parse->db;
> -	int i, j;		/* Loop counters */
>   	u32 cnt;		/* Index added to make the name unique */
> -	Column *aCol, *pCol;	/* For looping over result columns */
> -	int nCol;		/* Number of columns in the result set */
>   	Expr *p;		/* Expression for a single result column */
>   	char *zName;		/* Column name */
>   	int nName;		/* Size of name in zName[] */
>   	Hash ht;		/* Hash table of column names */
>   
>   	sqlite3HashInit(&ht);
> -	if (expr_list) {
> -		nCol = expr_list->nExpr;
> -		aCol = sqlite3DbMallocZero(db, sizeof(aCol[0]) * nCol);
> -		testcase(aCol == 0);
> -	} else {
> -		nCol = 0;
> -		aCol = NULL;
> -	}
> -	assert(nCol == (i16) nCol);
> +	uint32_t column_count =
> +		(expr_list != NULL) ? (uint32_t)expr_list->nExpr : 0;

6. Why do you need to enclose != NULL into the brackets?

>   	/*
>   	 * This should be a table without resolved columns.
>   	 * sqlite3ViewGetColumnNames could use it to resolve