[tarantool-patches] Re: [PATCH 3/3] sql: change collation compatibility rules
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Oct 31 15:34:53 MSK 2018
Thanks for the patch! See 5 comments below.
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index a13de4f0e..bd25ed656 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -316,25 +315,62 @@ binaryCompareP5(Expr * pExpr1, Expr * pExpr2, int jumpIfNull)
> return aff;
> }
>
> +int
> +collations_are_compatible(uint32_t lhs_id, bool is_lhs_forced,
> + uint32_t rhs_id, bool is_rhs_forced,
> + uint32_t *res_id)
1. Why not to rename to _check_compatibility() and set
diag right here?
> +{
> + assert(res_id != NULL);
> + if (is_lhs_forced && is_rhs_forced) {
> + if (lhs_id != rhs_id)
> + return -1;
> + }
> + if (is_lhs_forced) {
> + *res_id = lhs_id;
> + return 0;
> + }
> + if (is_rhs_forced) {
> + *res_id = rhs_id;
> + return 0;
> + }
> + if (lhs_id != rhs_id) {
> + if (lhs_id == COLL_NONE) {
> + *res_id = rhs_id;
> + return 0;
> + }
> + if (rhs_id == COLL_NONE) {
> + *res_id = lhs_id;
> + return 0;
> + }
> + return -1;
> + }
> + *res_id = lhs_id;
> + return 0;
> +}
> +
> struct coll *
> sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right,
> uint32_t *coll_id)
> {
> - struct coll *coll;
> - bool is_found;
> assert(left != NULL);
> - if ((left->flags & EP_Collate) != 0) {
> - coll = sql_expr_coll(parser, left, &is_found, coll_id);
> - } else if (right != NULL && (right->flags & EP_Collate) != 0) {
> - coll = sql_expr_coll(parser, right, &is_found, coll_id);
> - } else {
> - coll = sql_expr_coll(parser, left, &is_found, coll_id);
> - if (! is_found) {
> - coll = sql_expr_coll(parser, right, &is_found,
> - coll_id);
> - }
> + bool is_lhs_forced;
> + bool is_rhs_forced;
> + uint32_t lhs_coll_id;
> + uint32_t rhs_coll_id;
> + struct coll *lhs_coll = sql_expr_coll(parser, left, &is_lhs_forced,
> + &lhs_coll_id);
> + struct coll *rhs_coll = sql_expr_coll(parser, right, &is_rhs_forced,
> + &rhs_coll_id);
> + if (collations_are_compatible(lhs_coll_id, is_lhs_forced,
> + rhs_coll_id, is_rhs_forced,
> + coll_id) != 0) {
> + diag_set(ClientError, ER_ILLEGAL_COLLATION_MIX);
> + parser->rc = SQL_TARANTOOL_ERROR;
> + parser->nErr++;
> + *coll_id = COLL_NONE;
3. Why do you need to set coll_id in a case of an error? As I
understand, after an error the compilation is terminated.
> + return NULL;
> }
> - return coll;
> + return *coll_id == rhs_coll_id ? rhs_coll : lhs_coll;;
> }
>
> /*
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 1ec92aac7..c3403670b 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -2035,8 +2035,9 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */
> bool is_found;
> uint32_t coll_id;
> if (pTab->def->fields[i].coll_id == COLL_NONE &&
> - sql_expr_coll(pParse, p, &is_found, &coll_id) && is_found)
> + sql_expr_coll(pParse, p, &is_found, &coll_id) && coll_id != COLL_NONE)
> pTab->def->fields[i].coll_id = coll_id;
> + assert(pTab->def->fields[i].coll_id != COLL_BINARY);
4. Why? I can create a table with binary collation after the
previous commit.
> }
> }
>
> @@ -2223,47 +2224,53 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
> +static uint32_t
> +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
> + bool *is_forced_coll)
> {
> - struct coll *coll;
> + bool is_prior_forced = false;
> + bool is_current_forced;
> + uint32_t prior_coll_id = COLL_NONE;
> + uint32_t current_coll_id;
> if (p->pPrior != NULL) {
> - coll = multi_select_coll_seq_r(parser, p->pPrior, n, is_found,
> - coll_id);
> - } else {
> - coll = NULL;
> - *coll_id = COLL_NONE;
> + prior_coll_id = multi_select_coll_seq(parser, p->pPrior, n,
> + &is_prior_forced);
> }
> - assert(n >= 0);
> - /* iCol must be less than p->pEList->nExpr. Otherwise an error would
> - * have been thrown during name resolution and we would not have gotten
> - * this far
> + /*
> + * Column number must be less than p->pEList->nExpr.
> + * Otherwise an error would have been thrown during name
> + * resolution and we would not have gotten this far.
5. 'have gotten'? Maybe 'have got'?
> */
> - if (!*is_found && ALWAYS(n < p->pEList->nExpr)) {
> - coll = sql_expr_coll(parser, p->pEList->a[n].pExpr, is_found,
> - coll_id);
> - }
> - return coll;
> -}
More information about the Tarantool-patches
mailing list