[tarantool-patches] Re: [PATCH 3/6] sql: pass true types of columns to Tarantool

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

See 3 comments below.

On 17/09/2018 23:32, Nikita Pettik wrote:
> From: Georgy Kirichenko <georgy@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')
>   ---
>   ...
> +---
> +...
> +---
> +...