Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: v.shpilevoy@tarantool.org, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v1 3/8] sql: refactor UPPER() and LOWER() functions
Date: Wed, 20 Oct 2021 20:02:49 +0300
Message-ID: <20211020170249.GC203963@tarantool.org> (raw)
In-Reply-To: <7ac6109c37c1bd073a3693010ef7eaecf14074a8.1633105483.git.imeevma@gmail.com>

Thank you for the review! I replaced check of MEM type by mem_is_*() functions.
Diff below.

On Fri, Oct 01, 2021 at 07:29:29PM +0300, Mergen Imeev via Tarantool-patches wrote:
> Part of #4145
> ---
>  src/box/sql/func.c | 118 +++++++++++++++++++++------------------------
>  1 file changed, 55 insertions(+), 63 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 2e53b32d8..565ebcabe 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -295,6 +295,59 @@ func_char_length(struct sql_context *ctx, int argc, struct Mem *argv)
>  	mem_set_uint(ctx->pOut, utf8_len_str(arg->z, arg->n));
>  }
>  
> +/** Implementation of the UPPER() and LOWER() functions. */
> +static void
> +func_lower_upper(struct sql_context *ctx, int argc, struct Mem *argv)
> +{
> +	assert(argc == 1);
> +	(void)argc;
> +	struct Mem *arg = &argv[0];
> +	if (arg->type == MEM_TYPE_NULL)
> +		return;
> +	assert(arg->type == MEM_TYPE_STR && arg->n >= 0);
> +	if (arg->n == 0)
> +		return mem_set_str0_static(ctx->pOut, "");
> +	const char *str = arg->z;
> +	int32_t len = arg->n;
> +	struct sql *db = sql_get();
> +	char *res = sqlDbMallocRawNN(db, len);
> +	if (res == NULL) {
> +		ctx->is_aborted = true;
> +		return;
> +	}
> +	int32_t size = sqlDbMallocSize(db, res);
> +	assert(size >= len);
> +	UErrorCode status = U_ZERO_ERROR;
> +	const char *locale = NULL;
> +	if (ctx->coll != NULL && ctx->coll->type == COLL_TYPE_ICU) {
> +		locale = ucol_getLocaleByType(ctx->coll->collator,
> +					      ULOC_VALID_LOCALE, &status);
> +	}
> +	UCaseMap *cm = ucasemap_open(locale, 0, &status);
> +	assert(cm != NULL);
> +	assert(ctx->func->def->name[0] == 'U' ||
> +	       ctx->func->def->name[0] == 'L');
> +	bool is_upper = ctx->func->def->name[0] == 'U';
> +	int32_t new_len =
> +		is_upper ?
> +		ucasemap_utf8ToUpper(cm, res, size, str, len, &status) :
> +		ucasemap_utf8ToLower(cm, res, size, str, len, &status);
> +	if (new_len > size) {
> +		res = sqlDbRealloc(db, res, new_len);
> +		if (db->mallocFailed != 0) {
> +			ctx->is_aborted = true;
> +			return;
> +		}
> +		status = U_ZERO_ERROR;
> +		if (is_upper)
> +			ucasemap_utf8ToUpper(cm, res, size, str, len, &status);
> +		else
> +			ucasemap_utf8ToLower(cm, res, size, str, len, &status);
> +	}
> +	ucasemap_close(cm);
> +	mem_set_str_allocated(ctx->pOut, res, new_len);
> +}
> +
>  static const unsigned char *
>  mem_as_ustr(struct Mem *mem)
>  {
> @@ -808,67 +861,6 @@ contextMalloc(struct sql_context *context, i64 nByte)
>  	return z;
>  }
>  
> -/*
> - * Implementation of the upper() and lower() SQL functions.
> - */
> -
> -#define ICU_CASE_CONVERT(case_type)                                            \
> -static void                                                                    \
> -case_type##ICUFunc(sql_context *context, int argc, struct Mem *argv)           \
> -{                                                                              \
> -	char *z1;                                                              \
> -	const char *z2;                                                        \
> -	int n;                                                                 \
> -	UNUSED_PARAMETER(argc);                                                \
> -	if (mem_is_bin(&argv[0]) || mem_is_map(&argv[0]) ||                    \
> -	    mem_is_array(&argv[0])) {                                          \
> -		diag_set(ClientError, ER_INCONSISTENT_TYPES, "string",         \
> -			 mem_str(&argv[0]));                                   \
> -		context->is_aborted = true;                                    \
> -		return;                                                        \
> -	}                                                                      \
> -	z2 = mem_as_str0(&argv[0]);                                            \
> -	n = mem_len_unsafe(&argv[0]);                                          \
> -	/*                                                                     \
> -	 * Verify that the call to _bytes()                                    \
> -	 * does not invalidate the _text() pointer.                            \
> -	 */                                                                    \
> -	assert(z2 == mem_as_str0(&argv[0]));                                   \
> -	if (!z2)                                                               \
> -		return;                                                        \
> -	z1 = contextMalloc(context, ((i64) n) + 1);                            \
> -	if (z1 == NULL) {                                                      \
> -		context->is_aborted = true;                                    \
> -		return;                                                        \
> -	}                                                                      \
> -	UErrorCode status = U_ZERO_ERROR;                                      \
> -	struct coll *coll = context->coll;                                     \
> -	const char *locale = NULL;                                             \
> -	if (coll != NULL && coll->type == COLL_TYPE_ICU) {                     \
> -		locale = ucol_getLocaleByType(coll->collator,                  \
> -					      ULOC_VALID_LOCALE, &status);     \
> -	}                                                                      \
> -	UCaseMap *case_map = ucasemap_open(locale, 0, &status);                \
> -	assert(case_map != NULL);                                              \
> -	int len = ucasemap_utf8To##case_type(case_map, z1, n, z2, n, &status); \
> -	if (len > n) {                                                         \
> -		status = U_ZERO_ERROR;                                         \
> -		sql_free(z1);                                              \
> -		z1 = contextMalloc(context, ((i64) len) + 1);                  \
> -		if (z1 == NULL) {                                              \
> -			context->is_aborted = true;                            \
> -			return;                                                \
> -		}                                                              \
> -		ucasemap_utf8To##case_type(case_map, z1, len, z2, n, &status); \
> -	}                                                                      \
> -	ucasemap_close(case_map);                                              \
> -	sql_result_text(context, z1, len, sql_free);                   \
> -}                                                                              \
> -
> -ICU_CASE_CONVERT(Lower);
> -ICU_CASE_CONVERT(Upper);
> -
> -
>  /*
>   * Some functions like COALESCE() and IFNULL() and UNLIKELY() are implemented
>   * as VDBE code so that unused argument values do not have to be computed.
> @@ -2007,7 +1999,7 @@ static struct sql_func_definition definitions[] = {
>  	 FIELD_TYPE_BOOLEAN, sql_builtin_stub, NULL},
>  	{"LIKELY", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_BOOLEAN, sql_builtin_stub,
>  	 NULL},
> -	{"LOWER", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, LowerICUFunc,
> +	{"LOWER", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, func_lower_upper,
>  	 NULL},
>  
>  	{"MAX", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax, NULL},
> @@ -2082,7 +2074,7 @@ static struct sql_func_definition definitions[] = {
>  	 NULL},
>  	{"UNLIKELY", 1, {FIELD_TYPE_ANY}, FIELD_TYPE_BOOLEAN, sql_builtin_stub,
>  	 NULL},
> -	{"UPPER", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, UpperICUFunc,
> +	{"UPPER", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, func_lower_upper,
>  	 NULL},
>  	{"UUID", 0, {}, FIELD_TYPE_UUID, sql_func_uuid, NULL},
>  	{"UUID", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_UUID, sql_func_uuid, NULL},
> -- 
> 2.25.1
> 

Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 441bcb51d..d6c557c54 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -286,9 +286,9 @@ func_lower_upper(struct sql_context *ctx, int argc, struct Mem *argv)
 	assert(argc == 1);
 	(void)argc;
 	struct Mem *arg = &argv[0];
-	if (arg->type == MEM_TYPE_NULL)
+	if (mem_is_null(arg))
 		return;
-	assert(arg->type == MEM_TYPE_STR && arg->n >= 0);
+	assert(mem_is_str(arg) && arg->n >= 0);
 	if (arg->n == 0)
 		return mem_set_str0_static(ctx->pOut, "");
 	const char *str = arg->z;

  reply	other threads:[~2021-10-20 17:02 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-01 16:29 [Tarantool-patches] [PATCH v1 0/8] Rework standard function Mergen Imeev via Tarantool-patches
2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 1/8] sql: refactor ABS() funcion Mergen Imeev via Tarantool-patches
2021-10-08 21:55   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-20 16:52     ` Mergen Imeev via Tarantool-patches
2021-10-28 22:11       ` Vladislav Shpilevoy via Tarantool-patches
2021-11-01 10:11         ` Mergen Imeev via Tarantool-patches
2021-11-01 13:37           ` Vladislav Shpilevoy via Tarantool-patches
2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 2/8] sql: refactor CHAR_LENGTH() function Mergen Imeev via Tarantool-patches
2021-10-08 21:56   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-20 16:58     ` Mergen Imeev via Tarantool-patches
2021-10-28 22:11       ` Vladislav Shpilevoy via Tarantool-patches
2021-11-01 10:20         ` Mergen Imeev via Tarantool-patches
2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 3/8] sql: refactor UPPER() and LOWER() functions Mergen Imeev via Tarantool-patches
2021-10-20 17:02   ` Mergen Imeev via Tarantool-patches [this message]
2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 4/8] sql: refactor NULLIF() function Mergen Imeev via Tarantool-patches
2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 5/8] sql: rework TRIM() function Mergen Imeev via Tarantool-patches
2021-10-20 17:05   ` Mergen Imeev via Tarantool-patches
2021-10-28 22:12     ` Vladislav Shpilevoy via Tarantool-patches
2021-11-01 10:35       ` Mergen Imeev via Tarantool-patches
2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 6/8] sql: rework POSITION() function Mergen Imeev via Tarantool-patches
2021-10-08 21:58   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-20 17:08     ` Mergen Imeev via Tarantool-patches
2021-11-01 10:41       ` Mergen Imeev via Tarantool-patches
2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 7/8] sql: rework SUBSTR() function Mergen Imeev via Tarantool-patches
2021-10-08 22:02   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-20 17:15     ` Mergen Imeev via Tarantool-patches
2021-10-28 22:13       ` Vladislav Shpilevoy via Tarantool-patches
2021-11-01 10:45         ` Mergen Imeev via Tarantool-patches
2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 8/8] sql: refactor LIKE() function Mergen Imeev via Tarantool-patches
2021-10-08 22:02   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-20 17:19     ` Mergen Imeev via Tarantool-patches
2021-11-01 10:48       ` Mergen Imeev via Tarantool-patches
2021-11-01 10:53         ` Mergen Imeev via Tarantool-patches
2021-10-04 13:32 ` [Tarantool-patches] [PATCH v1 0/8] Rework standard function Mergen Imeev via Tarantool-patches
2021-11-01 13:38 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-11 10:45 Mergen Imeev via Tarantool-patches
2021-11-11 10:45 ` [Tarantool-patches] [PATCH v1 3/8] sql: refactor UPPER() and LOWER() functions Mergen Imeev via Tarantool-patches

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=20211020170249.GC203963@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git