Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Nikita Pettik <korablev@tarantool.org>, tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v2 3/3] sql: change collation compatibility rules
Date: Tue, 13 Nov 2018 15:02:41 +0300	[thread overview]
Message-ID: <105845b9-019b-9b37-4812-898c0fa4d4be@tarantool.org> (raw)
In-Reply-To: <a80af6ab287482aa8ea126bd06263fbc1f4ebe39.1542066357.git.korablev@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.

  reply	other threads:[~2018-11-13 12:02 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-13  0:07 [tarantool-patches] [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Nikita Pettik
2018-11-13  0:07 ` [tarantool-patches] [PATCH v2 1/3] sql: do not add explicit <COLLATE "BINARY"> clause Nikita Pettik
2018-11-13  0:07 ` [tarantool-patches] [PATCH v2 2/3] Introduce "none" and "binary" collations Nikita Pettik
2018-11-13 12:02   ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-13 22:37     ` n.pettik
2018-11-14 11:52       ` Vladislav Shpilevoy
2018-11-14 20:59         ` n.pettik
2018-11-15 11:04           ` Vladislav Shpilevoy
2018-11-13  0:07 ` [tarantool-patches] [PATCH v2 3/3] sql: change collation compatibility rules Nikita Pettik
2018-11-13 12:02   ` Vladislav Shpilevoy [this message]
2018-11-13 22:37     ` [tarantool-patches] " n.pettik
2018-11-15 11:24 ` [tarantool-patches] Re: [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=105845b9-019b-9b37-4812-898c0fa4d4be@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v2 3/3] sql: change collation compatibility rules' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox