[tarantool-patches] Re: [PATCH v2 3/3] sql: change collation compatibility rules

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

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@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.