[tarantool-patches] Re: [PATCH 3/6] sql: pass true types of columns to Tarantool
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Sep 27 23:23:57 MSK 2018
See 3 comments below.
On 17/09/2018 23:32, Nikita Pettik wrote:
> From: Georgy Kirichenko <georgy at tarantool.org>
>
> As a main part of introducing strict typing in SQL it is required to
> prohibit typeless columns in parser's grammar. Originally, SQLite simply
> assigns typeless columns to BLOB affinity. Moreover, due to historical
> reasons, all columns were stored with <SCALAR> type in Tarantool core
> (except for <INTEGER> when it comes to primary key). Column type should
> be defined on table creation. Allowed data types are: <TEXT>, <VARCHAR>,
> <CHAR>, <BLOB>, <INT[EGER]>, <REAL>, <FLOAT>, <NUMERIC>, <DECIMAL>,
> <DOUBLE> <DATE> and <DATETIME>. However, still any declared data type is
> converted to one of <BLOB>, <TEXT>, <REAL> or <INTEGER> affinities.
> While affinity reaches space format, it is (again) converted to
> Tarantool's field type. To be more precise, table of conversions:
>
> +----------+----------+------------+
> | SQL TYPE | AFFINITY | FIELD TYPE |
> +----------+----------+------------+
> | FLOAT | REAL | NUMBER |
> | REAL | REAL | NUMBER |
> | DOUBLE | REAL | NUMBER |
> | NUMERIC | REAL | NUMBER |
> | DECIMAL | REAL | NUMBER |
> | INTEGER | INTEGER | INTEGER |
> | TEXT | TEXT | STRING |
> | VARCHAR | TEXT | STRING |
> | CHAR | TEXT | STRING |
> | BLOB | BLOB | SCALAR |
> | DATETIME | REAL | NUMBER |
> | DATE | REAL | NUMBER |
> | TIME | REAL | NUMBER |
> +----------+----------+------------+
>
> <VARCHAR> and <CHAR> types should be specified with length
> (e.g. name VARCHAR(10)), but this length currently is not used when
> types are processed. Only purpose is to support ANSI syntax.
> The same for <NUMERIC> and <DECIMAL> - it is allowed to specify scale
> and precision, but they don't affect the way they are stored in memory.
>
> Note that patch is not self-sufficient: a lot of tests still fail due to
> wrong types conversions. Fix for that comes as next patch.
>
> Closes #3018
> Closes #3104
> Closes #2494
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index b57e3811d..cde6bf996 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -99,6 +99,22 @@ sqlite3ExprAffinity(Expr * pExpr)
> return sqlite3ExprAffinity(pExpr->pLeft->x.pSelect->pEList->
> a[pExpr->iColumn].pExpr);
> }
> + if (op == TK_PLUS) {
> + assert(pExpr->pRight != NULL && pExpr->pLeft != NULL);
> + enum affinity_type lhs_aff = sqlite3ExprAffinity(pExpr->pLeft);
> + enum affinity_type rhs_aff = sqlite3ExprAffinity(pExpr->pRight);
> + return sql_affinity_result(rhs_aff, lhs_aff);
> + }
> + if (op == TK_COLUMN) {
> + assert(pExpr->space_def != NULL);
> + const char *col_name = pExpr->u.zToken;
> + size_t name_len = strlen(col_name);
> + uint32_t field_no;
> + tuple_fieldno_by_name(pExpr->space_def->dict, col_name, name_len,
> + field_name_hash(col_name, name_len),
> + &field_no);
1. Why are you sure that tuple_fieldno_by_name can not
return an error?
2. This code is unreachable - in the same function on line 93 TK_COLUMN
is processed already.
What is curious - I removed this hunk entirely and next commit's tests
passed. So op == TK_PLUS check here is not needed as well, is it? I guess,
it is already processed in another place.
> + return pExpr->space_def->fields[field_no].affinity;
> + }
> return pExpr->affinity;
> }
>
> diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result
> index 63fe48e79..290aa4162 100644
> --- a/test/sql/on-conflict.result
> +++ b/test/sql/on-conflict.result
> @@ -1,9 +1,17 @@
> test_run = require('test_run').new()
> ---
> ...
> +---
> +...
> +---
> +...
3. wtf ???
> engine = test_run:get_cfg('engine')
> ---
> ...
> +---
> +...
> +---
> +...
More information about the Tarantool-patches
mailing list