[tarantool-patches] Re: [PATCH v4 4/7] sql: start using collations and is_nullable from space_def

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

Hello. See 5 comments below.

On 28/04/2018 21:26, Kirill Shcherbatov wrote:
> Part of #3272.
> ---
>   src/box/sql.c           | 36 +++++++++++++++++++++++++-----------
>   src/box/sql/alter.c     | 11 ++++++++---
>   src/box/sql/build.c     | 20 +++++++++++++-------
>   src/box/sql/fkey.c      |  7 ++-----
>   src/box/sql/select.c    |  6 ++++--
>   src/box/sql/sqliteInt.h |  7 -------
>   src/box/sql/vdbeaux.c   |  5 ++++-
>   7 files changed, 56 insertions(+), 36 deletions(-)
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 2893d70..ef11eb9 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1449,7 +1449,9 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
>   
>   	for (i = 0; i < n; i++) {
>   		const char *t;
> -		struct coll *coll = aCol[i].coll;
> +		struct coll *coll =
> +			coll_by_id(pTable->def->fields[i].coll_id);
> +
>   		struct field_def *field = &def->fields[i];
>   		const char *zToken = field->default_value;
>   		int base_len = 4;
> @@ -1461,19 +1463,25 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
>   		p = enc->encode_str(p, "name", 4);
>   		p = enc->encode_str(p, field->name, strlen(field->name));
>   		p = enc->encode_str(p, "type", 4);
> +
> +		assert(def->fields[i].is_nullable ==
> +		       (def->fields[i].nullable_action ==
> +			ON_CONFLICT_ACTION_NONE));

1. AFAIR I asked you to wrap this check into a function.
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index bedf602..f830a15 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -307,9 +312,9 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
>   	}
>   	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: Column *pCol = &pNew->aCol[i]; */

2. Why FIXME ?
> diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
> index c15ad8c..8c015c9 100644
> --- a/src/box/sql/fkey.c
> +++ b/src/box/sql/fkey.c
> @@ -34,6 +34,7 @@
>    * support to compiled SQL statements.
>    */
>   #include <box/coll.h>
> +#include "box/coll_cache.h"

3. Why? It is not used here.
> @@ -535,11 +536,7 @@ exprTableRegister(Parse * pParse,	/* Parsing and code generating context */
>   			pCol = &pTab->aCol[iCol];
>   			pExpr->iTable = regBase + iCol + 1;
>   			pExpr->affinity = pCol->affinity;
> -			const char *coll_name;
> -			if (pCol->coll == NULL && pCol->coll != NULL)
> -				coll_name = pCol->coll->name;
> -			else
> -				coll_name = "binary";
> +			const char *coll_name = "binary";
>   			pExpr = sqlite3ExprAddCollateString(pParse, pExpr,
>   							    coll_name);

4. Why you did not just inline "binary" string ?
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 03bfcf9..fa1de9b 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1836,6 +1836,8 @@ sqlite3ColumnsFromExprList(Parse * pParse,	/* Parsing context */
>   	pTable->def->fields =
>   		region_alloc(region, nCol*sizeof(pTable->def->fields[0]));
>   	memset(pTable->def->fields, 0, nCol*sizeof(pTable->def->fields[0]));
> +	for (int i = 0; i < nCol; i++)
> +		pTable->def->fields[i].is_nullable = true;

5. Why?