[tarantool-patches] Re: [PATCH v4 4/7] sql: start using collations and is_nullable from space_def
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu May 3 13:21:06 MSK 2018
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?
More information about the Tarantool-patches
mailing list