[tarantool-patches] Re: [PATCH v2 3/3] sql: change collation compatibility rules
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Nov 13 15:02:41 MSK 2018
Thanks for the fixes!
See 2 comments below, my fixes at the end
of the email and on the branch.
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index cea453f08..ab0376a1d 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -2150,47 +2151,51 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
> }
>
> #ifndef SQLITE_OMIT_COMPOUND_SELECT
> -static struct coll *
> -multi_select_coll_seq_r(Parse *parser, Select *p, int n, bool *is_found,
> - uint32_t *coll_id)
> +/**
> + * This function determines resulting collation sequence for
> + * @n-th column of the result set for the compound SELECT
> + * statement. Since compound SELECT performs implicit comparisons
> + * between values, all parts of compound queries must use
> + * the same collation. Otherwise, an error is raised.
> + *
> + * @param parser Parse context.
> + * @param p Select meta-information.
> + * @param n Column number of the result set.
> + * @param is_forced_coll Used if we fall into recursion.
> + * For most-outer call it is unused. Used to indicate that
> + * explicit COLLATE clause is used.
> + * @retval Id of collation to be used during string comparison.
> + */
> +static uint32_t
> +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
> + bool *is_forced_coll)
1. I renamed this function into multi_select_coll_seq_r and
created a wrapper without is_forced_coll to remove some of
'bool unused' things. Apply or drop the fix, up to you.
> diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
> index 9fa6ce15d..e05967554 100644
> --- a/src/box/sql/whereexpr.c
> +++ b/src/box/sql/whereexpr.c
> @@ -168,7 +168,7 @@ exprCommute(Parse * pParse, Expr * pExpr)
> bool is_found;
> uint32_t id;
> sql_expr_coll(pParse, pExpr->pLeft, &is_found, &id);
> - if (is_found) {
> + if (id != 0) {
2. If you fix a comment about COLL_NONE constant return,
do not forget to pick this hunk up as well, please. Maybe
this patch contains more, I am not sure.
> /*
> * Neither X nor Y have COLLATE
> * operators, but X has a
====================================================================
commit b138616a1851dbe905a2e86b981f36793a7eb117
Author: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
Date: Tue Nov 13 14:39:45 2018 +0300
Speculative review fixes
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index ffbe83aa5..2db8e1c7f 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2167,16 +2167,16 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
* @retval Id of collation to be used during string comparison.
*/
static uint32_t
-multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
- bool *is_forced_coll)
+multi_select_coll_seq_r(struct Parse *parser, struct Select *p, int n,
+ bool *is_forced_coll)
{
bool is_prior_forced = false;
bool is_current_forced;
uint32_t prior_coll_id = 0;
uint32_t current_coll_id;
if (p->pPrior != NULL) {
- prior_coll_id = multi_select_coll_seq(parser, p->pPrior, n,
- &is_prior_forced);
+ prior_coll_id = multi_select_coll_seq_r(parser, p->pPrior, n,
+ &is_prior_forced);
}
/*
* Column number must be less than p->pEList->nExpr.
@@ -2198,6 +2198,13 @@ multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
return res_coll_id;
}
+static inline uint32_t
+multi_select_coll_seq(struct Parse *parser, struct Select *p, int n)
+{
+ bool unused;
+ return multi_select_coll_seq_r(parser, p, n, &unused);
+}
+
/**
* The select statement passed as the second parameter is a
* compound SELECT with an ORDER BY clause. This function
@@ -2237,8 +2244,7 @@ sql_multiselect_orderby_to_key_info(struct Parse *parse, struct Select *s,
sql_expr_coll(parse, term, &unused, &id);
} else {
id = multi_select_coll_seq(parse, s,
- item->u.x.iOrderByCol - 1,
- &unused);
+ item->u.x.iOrderByCol - 1);
if (id != COLL_NONE) {
const char *name = coll_by_id(id)->name;
order_by->a[i].pExpr =
@@ -2900,10 +2906,9 @@ multiSelect(Parse * pParse, /* Parsing context */
struct sql_key_info *key_info = sql_key_info_new(db, nCol);
if (key_info == NULL)
goto multi_select_end;
- bool unused;
for (int i = 0; i < nCol; i++) {
key_info->parts[i].coll_id =
- multi_select_coll_seq(pParse, p, i, &unused);
+ multi_select_coll_seq(pParse, p, i);
}
for (struct Select *pLoop = p; pLoop; pLoop = pLoop->pPrior) {
@@ -3328,12 +3333,10 @@ multiSelectOrderBy(Parse * pParse, /* Parsing context */
pParse->nMem += expr_count + 1;
sqlite3VdbeAddOp2(v, OP_Integer, 0, regPrev);
key_info_dup = sql_key_info_new(db, expr_count);
- bool unused;
if (key_info_dup != NULL) {
for (int i = 0; i < expr_count; i++) {
key_info_dup->parts[i].coll_id =
- multi_select_coll_seq(pParse, p, i,
- &unused);
+ multi_select_coll_seq(pParse, p, i);
}
}
}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index cfe87c62a..470320e8d 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4330,7 +4330,8 @@ const char *sqlite3ErrStr(int);
*
* @param parse Parsing context.
* @param expr Expression to scan.
- * @param[out] is_found Flag set if explicit COLLATE clause is used.
+ * @param[out] is_explicit_coll Flag set if explicit COLLATE
+ * clause is used.
* @param[out] coll_id Collation identifier.
*
* @retval Pointer to collation.
More information about the Tarantool-patches
mailing list