[tarantool-patches] Re: [PATCH 2/2] sql: compute resulting collation for concatenation

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

Thanks for the patch! See 7 comments below.

On 16/01/2019 16:13, Nikita Pettik wrote:
> According to ANSI, result of concatenation operation should derive
> collation sequence from its operands. Now it is not true: result is
> always comes with no ("none") collation.
> 
> In a nutshell*, rules are quite simple:

1. If you want to put a reference, then use [<number>], like in
literature and like Vova does in his commits. Firstly, I thought
that '*' was a typo.

> 
> a) If some data type  has an explicit collation EC1, then every data

2. Double space after 'type'. In some other places below too.

> type that has an explicit collation shall have declared type collation
> that is EC1.  The collation derivation is explicit and the collation is
> EC1.
> 
> b) If every data type has an implicit collation, then:
> 
>   - If every data type has the same declared type collation IC1, then
>     the collation derivation is implicit and the collation is IC1.
> 
>   - Otherwise, the collation derivation is none.
> 
> c) Otherwise, the collation derivation is none.
> 
> *Read complete statement at 9.5 Result of data type combinations

3. Please, say a bit more words: that 9.5 is a chapter in an SQL
standard, and the standard of which year it is.

> 
> Closes #3937
> ---
>   src/box/sql/expr.c          |  47 +++++++++++++++++++-
>   test/sql/collation.result   | 102 ++++++++++++++++++++++++++++++++++++++++++++
>   test/sql/collation.test.lua |  46 ++++++++++++++++++++
>   3 files changed, 193 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index f8819f779..e6f536757 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -221,6 +221,45 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
>   			}
>   			break;
>   		}
> +		if (op == TK_CONCAT) {
> +			/*
> +			 * Despite the fact that procedure below
> +			 * is very similar to collation_check_compatability(),
> +			 * it is slightly different: when both
> +			 * operands have different implicit collations,
> +			 * derived collation should be "none",
> +			 * i.e. no collation is used at all
> +			 * (instead of raising error).

4. Typo: collation_check_compatability -> collation*S*_check_compat*I*bility.

Also, I think that it is not worth mentioning the difference here
with that function, especially in such a big comment, looks like an excuse.
It is better to put a link to the standard.

> +			 */
> +			bool is_lhs_forced;
> +			uint32_t lhs_coll_id;
> +			if (sql_expr_coll(parse, p->pLeft, &is_lhs_forced,
> +					  &lhs_coll_id) != 0)
> +				return -1;
> +			bool is_rhs_forced;
> +			uint32_t rhs_coll_id;
> +			if (sql_expr_coll(parse, p->pRight, &is_rhs_forced,
> +					  &rhs_coll_id) != 0)
> +				return -1;
> +			if (is_lhs_forced && is_rhs_forced) {
> +				if (lhs_coll_id != rhs_coll_id)
> +					return -1;

5. Did you miss diag_set?

> +			}
> +			if (is_lhs_forced) {
> +				*coll_id = lhs_coll_id;
> +				*is_explicit_coll = true;
> +				return 0;

6. In this function (sql_expr_coll) to break the cycle 'break'
keyword is used, so lets be consistent and use 'break' as well.

> +			}
> +			if (is_rhs_forced) {
> +				*coll_id = rhs_coll_id;
> +				*is_explicit_coll = true;
> +				return 0;
> +			}
> +			if (rhs_coll_id != lhs_coll_id)
> +				return 0;
> +			*coll_id = lhs_coll_id;
> +			return 0;
> +		}
>   		if (p->flags & EP_Collate) {
>   			if (p->pLeft && (p->pLeft->flags & EP_Collate) != 0) {
>   				p = p->pLeft;
> @@ -384,10 +423,14 @@ sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right)
>   	bool is_rhs_forced;
>   	uint32_t lhs_coll_id;
>   	uint32_t rhs_coll_id;
> -	if (sql_expr_coll(parser, left, &is_lhs_forced, &lhs_coll_id) != 0)
> +	if (sql_expr_coll(parser, left, &is_lhs_forced, &lhs_coll_id) != 0) {
> +		diag_set(ClientError, ER_ILLEGAL_COLLATION_MIX);
>   		goto err;
> -	if (sql_expr_coll(parser, right, &is_rhs_forced, &rhs_coll_id) != 0)
> +	}
> +	if (sql_expr_coll(parser, right, &is_rhs_forced, &rhs_coll_id) != 0) {
> +		diag_set(ClientError, ER_ILLEGAL_COLLATION_MIX);

7. Why do you set it here and not on the point 5 above? sql_expr_coll
can return an error not only because of illegal collation mix, but also
if a collation does not exist, for example.

>   		goto err;
> +	}
>   	uint32_t coll_id;
>   	if (collations_check_compatibility(lhs_coll_id, is_lhs_forced,
>   					   rhs_coll_id, is_rhs_forced,