[tarantool-patches] Re: [PATCH] sql: modify TRIM() function signature

[Roman Khabibov <roman.habibov@tarantool.org>]

Hi! Thanks for the review.

> On Apr 19, 2019, at 3:49 PM, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the fixes, much better already,
> almost done! But see 16 comments below.
> 
>>>> diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
>>>> index 251cc3534..8fe04fab1 100755
>>>> --- a/test/sql-tap/func.test.lua
>>>> +++ b/test/sql-tap/func.test.lua
>>>> @@ -1,6 +1,6 @@
>>>> #!/usr/bin/env tarantool
>>>> test = require("sqltester")
>>>> -test:plan(14586)
>>>> +test:plan(14590)
>>>> 
>>>> --!./tcltestrunner.lua
>>>> -- 2001 September 15
>>>> @@ -1912,37 +1912,37 @@ test:do_test(
>>>> test:do_catchsql_test(
>>>>    "func-22.1",
>>>>    [[
>>>> -        SELECT trim(1,2,3)
>>>> +        SELECT TRIM(1,2,3)
>>> 
>>> 20. Why? I thought that all identifiers are normalized
>>> anyway, including function names, and you do not need
>>> to uppercase everything manually. The same about the
>>> test func-22.4, func-22.20.
>> For consistency.
> 
> 1. Please, do not make dubious changes not required by
> the patch, wiping the git history, and padding the diff
> out. Keep the old version. In other places, where the
> only change was uppercasing, too, please.
Understood.

> 
>>    sql: modify TRIM() function signature
>> 
>>    According to the ANSI standard, ltrim, rtrim and trim should
>>    be merged into one unified TRIM() function. The specialization of
>>    trimming (left, right or both and trimming characters) determined
>>    in arguments of this function.
>> 
>>    Closes #3879
>> 
>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
>> index abeecefa1..ac52ddda2 100644
>> --- a/src/box/sql/func.c
>> +++ b/src/box/sql/func.c
>> @@ -1286,108 +1286,183 @@ replaceFunc(sql_context * context, int argc, sql_value ** argv)
>> 	sql_result_text(context, (char *)zOut, j, sql_free);
>> }
>> 
>> -/*
>> - * Implementation of the TRIM(), LTRIM(), and RTRIM() functions.
>> - * The userdata is 0x1 for left trim, 0x2 for right trim, 0x3 for both.
>> +/**
>> + * Remove chars included into @a trimming set from @a input_str.
> 
> 2. There is no parameter named 'trimming'. According to
> doxygen documentation, @a takes one word as an argument
> and it is usually the function argument's name.
> http://www.doxygen.nl/manual/commands.html#cmda
> 
> Please, prune old parameter names from the comments.
+/**
+ * Remove characters included in @a trim_set from @a input_str
+ * until encounter a character that doesn't belong to @a trim_set.
+ * Remove from the side specified by @a flags.
+ * @param context SQL context.
+ * @param flags Trim specification: left, right or both.
+ * @param trim_set The set of characters for trimming.
+ * @param trim_set_sz Character set size in bytes.
+ * @param input_str Input string for trimming.
+ * @param input_str_sz Input string size in bytes.
  */

>> {
>> -	const unsigned char *zIn;	/* Input string */
>> -	const unsigned char *zCharSet;	/* Set of characters to trim */
>> -	int nIn;		/* Number of bytes in input */
>> -	int flags;		/* 1: trimleft  2: trimright  3: trim */
>> -	int i;			/* Loop counter */
>> -	unsigned char *aLen = 0;	/* Length of each character in zCharSet */
>> -	unsigned char **azChar = 0;	/* Individual characters in zCharSet */
>> -	int nChar;		/* Number of characters in zCharSet */
>> +	int i;
>> +	/* Length of each character in the character set. */
>> +	char unsigned *char_len = 0;
> 
> 4. You again ignored my comment about NULL. Please, find all other
> places and fix it finally. I said it already 1000 times in 1000
> reviews - we do not use 0 for pointers. It is a simple rule. Just
> follow it. Write it down somewhere in a list of code style rules
> and check them all before sending a patch.
> 
> Seeing how many my comments you repeatedly ignore, I think that
> probably you should reconsider the way how you do self-reviews. If
> you do it via just looking a couple of seconds at the code in the
> text editor, then it is definitely a bad way.
> 
> First of all, use 'git diff/show' in console to look only at the
> patch changes, not at the entire files and functions. If you do not
> like console, and it is ok, then you can use Sublime Merge
> desktop program or Sublime Git package for the editor. When you
> look at the diff only, it is much simpler to notice such violations
> and even bugs.
+	int char_cnt = sql_utf8_char_count(z, trim_set_sz);
+	if (char_cnt == 0)

+	unsigned char **ind_chars =
+		contextMalloc(context,
+			      char_cnt * (sizeof(unsigned char *) + 1));
+	if (ind_chars == NULL)

+	int i = 0;
+	char_cnt = 0;

+	if ((input_str = sql_value_text(argv[0])) == NULL)
+		return;

+	if ((input_str = sql_value_text(argv[1])) == NULL)
+		return;

+	} else if ((trim_set = sql_value_text(argv[0])) != NULL) {
+		int trim_set_sz = sql_value_bytes(argv[0]);

+	if ((input_str = sql_value_text(argv[2])) == NULL ||
+	    (trim_set = sql_value_text(argv[1])) == NULL)
+		return;

> 5. In our code style we do not use 'char' to represent numbers, we
> use 'uint8_t' or 'int8_t' when we want to use one-byte numbers. It
> is the same as 'char'/'unsigned char', but looks shorter and it
> becomes obvious that these values are used as numbers, not text.
> Firstly I thought that char_len was an array of characters, but
> it emerged being an array of symbol sizes. In the summary, I
> suggest to use 'uint8_t *' for char_len array.
+	uint8_t *char_len = (uint8_t *)&ind_chars[char_cnt];

>> +	/* Individual characters in the character set. */
>> +	char unsigned **ind_chars = 0;
> 
> 6. If you declare it as 'const char unsigned **', then you
> can remove unnecessary type cast from line 1330.
If you meant the following line "ind_chars[char_cnt] = (unsigned char *)(z + i);”
then it isn’t compiled without the cast, because of assigning to "'unsigned char *’
from 'const unsigned char *’”.
> 7. Normally, we do not reorder 'unsigned' and 'char/int/long'.
> 
>    char unsigned -> unsigned char
Sorry. Didn’t notice.

>> 
>> -	if (sql_value_type(argv[0]) == SQL_NULL) {
>> -		return;
>> -	}
>> -	zIn = sql_value_text(argv[0]);
>> -	if (zIn == 0)
>> -		return;
>> -	nIn = sql_value_bytes(argv[0]);
>> -	assert(zIn == sql_value_text(argv[0]));
>> -	if (argc == 1) {
>> -		static const unsigned char lenOne[] = { 1 };
>> -		static unsigned char *const azOne[] = { (u8 *) " " };
>> -		nChar = 1;
>> -		aLen = (u8 *) lenOne;
>> -		azChar = (unsigned char **)azOne;
>> -		zCharSet = 0;
>> -	} else if ((zCharSet = sql_value_text(argv[1])) == 0) {
>> -		return;
>> -	} else {
>> -		const unsigned char *z = zCharSet;
>> -		int trim_set_sz = sql_value_bytes(argv[1]);
>> -		/*
>> -		* Count the number of UTF-8 characters passing
>> -		* through the entire char set, but not up
>> -		* to the '\0' or X'00' character. This allows
>> -		* to handle trimming set containing such
>> -		* characters.
>> -		*/
>> -		nChar = sql_utf8_char_count(z, trim_set_sz);
>> -		if (nChar > 0) {
>> -			azChar =
>> -			    contextMalloc(context,
>> -					  ((i64) nChar) * (sizeof(char *) + 1));
>> -			if (azChar == 0) {
>> -				return;
>> -			}
>> -			aLen = (unsigned char *)&azChar[nChar];
>> -			z = zCharSet;
>> -			i = 0;
>> -			nChar = 0;
>> -			int handled_bytes_cnt = trim_set_sz;
>> -			while(handled_bytes_cnt > 0) {
>> -				azChar[nChar] = (unsigned char *)(z + i);
>> -				SQL_UTF8_FWD_1(z, i, trim_set_sz);
>> -				aLen[nChar] = (u8) (z + i - azChar[nChar]);
>> -				handled_bytes_cnt -= aLen[nChar];
>> -				nChar++;
>> -			}
>> +	const unsigned char *z = trim_set;
>> +	/*
>> +	 * Count the number of UTF-8 characters passing through
>> +	 * the entire char set, but not up to the '\0' or X'00'
>> +	 * character. This allows to handle trimming set
>> +	 * containing such characters.
>> +	 */
>> +	int char_cnt = sql_utf8_char_count(z, trim_set_sz);
>> +	if (char_cnt > 0) {
>> +		ind_chars =
>> +		    contextMalloc(context,
>> +				  ((i64) char_cnt) *

> 8. Why do you need that cast to 'i64'? Anyway you access that memory by
> 'int' indexes in the next lines. Please, remove it.
+	unsigned char **ind_chars =
+		contextMalloc(context,
+			      char_cnt * (sizeof(unsigned char *) + 1));

> 
>> +				  (sizeof(unsigned char *) + 1));
>> +		if (ind_chars == 0)
>> +			return;
>> +		char_len = (unsigned char *)&ind_chars[char_cnt];
>> +		z = trim_set;
>> +		i = 0;
>> +		char_cnt = 0;
>> +		int handled_bytes_cnt = trim_set_sz;
>> +		while(handled_bytes_cnt > 0) {
>> +			ind_chars[char_cnt] = (unsigned char *)(z + i);
>> +			SQL_UTF8_FWD_1(z, i, trim_set_sz);
>> +			char_len[char_cnt] = (u8) (z + i - ind_chars[char_cnt]);
> 9. Why do you need that cast to 'u8'? 'u8' == 'unsigned char', and the
> type of that expression is already 'unsigned char’.
+		char_len[char_cnt] = z + i - ind_chars[char_cnt];

>> +			handled_bytes_cnt -= char_len[char_cnt];
>> +			char_cnt++;
>> 		}
>> 	}
>> -	if (nChar > 0) {
>> -		flags = SQL_PTR_TO_INT(sql_user_data(context));
>> -		if (flags & 1) {
>> -			while (nIn > 0) {
>> +	if (char_cnt > 0) {
> 10. Indentation next 33 lines is huge and they are followed by
> just one 2-line function call. Just do 'goto result' here if
> char_cnt == 0 and reduce the indentation. The same can be done at
> line 1317 in order to reduce indentation of next 17 lines.
+	if (char_cnt == 0)
+		goto result;
+	/* Individual characters in the character set. */
+	unsigned char **ind_chars =
+		contextMalloc(context,
+			      char_cnt * (sizeof(unsigned char *) + 1));
+	if (ind_chars == NULL)
 		return;

+	if (char_cnt == 0)
+		goto result;
+	if ((flags & TRIM_LEADING) != 0) {
+		while (input_str_sz > 0) {

I have never used this operator before.

>> +		if ((flags & TRIM_LEADING) != 0) {
>> +			while (input_str_sz > 0) {
>> 				int len = 0;
>> -				for (i = 0; i < nChar; i++) {
>> -					len = aLen[i];
>> -					if (len <= nIn
>> -					    && memcmp(zIn, azChar[i], len) == 0)
>> +				for (i = 0; i < char_cnt; i++) {
>> +					len = char_len[i];
>> +					if (len <= input_str_sz
>> +					    && memcmp(input_str,
>> +						      ind_chars[i], len) == 0)
>> 						break;
>> 				}
>> -				if (i >= nChar)
>> +				if (i >= char_cnt)
>> 					break;
>> -				zIn += len;
>> -				nIn -= len;
>> +				input_str += len;
>> +				input_str_sz -= len;
>> 			}
>> 		}
>> -		if (flags & 2) {
>> -			while (nIn > 0) {
>> +		if ((flags & TRIM_TRAILING) != 0) {
>> +			while (input_str_sz > 0) {
>> 				int len = 0;
>> -				for (i = 0; i < nChar; i++) {
>> -					len = aLen[i];
>> -					if (len <= nIn
>> -					    && memcmp(&zIn[nIn - len],
>> -						      azChar[i], len) == 0)
>> +				for (i = 0; i < char_cnt; i++) {
>> +					len = char_len[i];
>> +					if (len <= input_str_sz
>> +					    && memcmp(&input_str[input_str_sz - len],
>> +						      ind_chars[i], len) == 0)
> 
> 11. Out of 80. And you saw that in your editor, even without
> 'git diff' and console, because you have 80-rulers. So why did
> you decide not to fix it?
I often saw same instances in the Tarantool’s code, when few characters is out of 80.
In my case, I just didn’t know how to fix that.

>> 						break;
>> 				}
>> -				if (i >= nChar)
>> +				if (i >= char_cnt)
>> 					break;
>> -				nIn -= len;
>> +				input_str_sz -= len;
>> 			}
>> 		}
>> -		if (zCharSet) {
>> -			sql_free(azChar);
>> -		}
>> +		if (trim_set_sz != 0)
>> +			sql_free(ind_chars);
>> +	}
>> +	sql_result_text(context, (char *)input_str, input_str_sz,
>> +			SQL_TRANSIENT);
>> +}
>> +
>> +/**
>> + * Normalize args from @a argv input array when it has one arg
>> + * only.
>> + *
>> + * Case: TRIM(<str>)
>> + * Call trimming procedure with TRIM_BOTH as the flags and " " as
>> + * the trimming set.
>> + *
>> + * @param context SQL context.
> 
> 12. As I said in the previous reviews, we either omit doxygen
> formal style completely, or use it correctly. If you want to use
> doxygen, please, describe all the 3 parameters. If you do not
> want, then omit @param/@retval section. The same for other places.
+/**
+ * Normalize args from @a argv input array when it has one arg
+ * only.
+ *
+ * Case: TRIM(<str>)
+ * Call trimming procedure with TRIM_BOTH as the flags and " " as
+ * the trimming set.
+ */
+static void
+trim_func_one_arg(struct sql_context *context, int argc, sql_value **argv)

+/**
+ * Normalize args from @a argv input array when it has two args.
+ *
+ * Case: TRIM(<character_set> FROM <str>)
+ * If user has specified <character_set> only, call trimming
+ * procedure with TRIM_BOTH as the flags and that trimming set.
+ *
+ * Case: TRIM(LEADING/TRAILING/BOTH FROM <str>)
+ * If user has specified side keyword only, then call trimming
+ * procedure with the specified side and " " as the trimming set.
+ */
+static void
+trim_func_two_args(struct sql_context *context, int argc, sql_value **argv)

+/**
+ * Normalize args from @a argv input array when it has three args.
+ *
+ * Case: TRIM(LEADING/TRAILING/BOTH <character_set> FROM <str>)
+ * If user has specified side keyword and <character_set>, then
+ * call trimming procedure with that args.
+ */
+static void
+trim_func_three_args(struct sql_context *context, int argc, sql_value **argv)

>> + */
>> +static void
>> +trim_func_one_arg(sql_context * context, int argc, sql_value **argv)
> 
> 13. In new code we use explicit 'struct' keyword for struct
> types - sql_context and sql_value. Also, we do not put whitepaces
> after '*' when declare a pointer type value. The same for other places.
+trim_procedure(struct sql_context *context, enum trim_side_mask flags,
+	       const unsigned char *trim_set, int trim_set_sz,
+	       const unsigned char *input_str, int input_str_sz)

+static void
+trim_func_one_arg(struct sql_context *context, int argc, sql_value **argv)

+static void
+trim_func_two_args(struct sql_context *context, int argc, sql_value **argv)

+static void
+trim_func_three_args(struct sql_context *context, int argc, sql_value **argv)

>> +/**
>> + * Normalize args from @a argv input array when it has two args.
>> + *
>> + * Case: TRIM(<character_set> FROM <str>)
>> + * If user has specified <character_set> only, call trimming
>> + * procedure with TRIM_BOTH as the flags and that trimming set.
>> + *
>> + * Case: TRIM(LEADING/TRAILING/BOTH FROM <str>)
>> + * If user has specified side keyword only, then call trimming
>> + * procedure with the specified side and " " as the trimming set.
>> + *
>> + * @param context SQL context.
>> + */
>> +static void
>> +trim_func_two_args(sql_context * context, int argc, sql_value **argv)
>> +{
>> +	const unsigned char *input_str;
>> +	assert(argc == 2);
>> +	(void) argc;
>> +
>> +	if ((input_str = sql_value_text(argv[1])) == NULL)
>> +		return;
>> +	int input_str_sz = sql_value_bytes(argv[1]);
>> +
>> +	const char unsigned *trim_set;
>> +	if (sql_value_type(argv[0]) == SQL_INTEGER) {
>> +		trim_procedure(context, sql_value_int(argv[0]),
>> +			       (const unsigned char *) " ", 1,
>> +			       input_str, input_str_sz);
>> +	} else if ((trim_set = sql_value_text(argv[0])) == NULL) {
>> +		return;
>> +	} else {
> 
> 14. Please, apply.
> 
> @@ -1427,9 +1427,7 @@ trim_func_two_args(sql_context * context, int argc, sql_value **argv)
> 		trim_procedure(context, sql_value_int(argv[0]),
> 			       (const unsigned char *) " ", 1,
> 			       input_str, input_str_sz);
> -	} else if ((trim_set = sql_value_text(argv[0])) == NULL) {
> -		return;
> -	} else {
> +	} else if ((trim_set = sql_value_text(argv[0])) != NULL) {
> 		int trim_set_sz = sql_value_bytes(argv[0]);
> 		trim_procedure(context, TRIM_BOTH, trim_set, trim_set_sz,
> 			       input_str, input_str_sz);
+static void
+trim_func_two_args(struct sql_context *context, int argc, sql_value **argv)
+{
+	assert(argc == 2);
+	(void) argc;
+
+	const unsigned char *input_str;
+	if ((input_str = sql_value_text(argv[1])) == NULL)
+		return;
+
+	int input_str_sz = sql_value_bytes(argv[1]);
+	const unsigned char *trim_set;
+	if (sql_value_type(argv[0]) == SQL_INTEGER) {
+		trim_procedure(context, sql_value_int(argv[0]),
+			       (const unsigned char *) " ", 1,
+			       input_str, input_str_sz);
+	} else if ((trim_set = sql_value_text(argv[0])) != NULL) {
+		int trim_set_sz = sql_value_bytes(argv[0]);
+		trim_procedure(context, TRIM_BOTH, trim_set, trim_set_sz,
+			       input_str, input_str_sz);
+	}
+}

>> +		int trim_set_sz = sql_value_bytes(argv[0]);
>> +		trim_procedure(context, TRIM_BOTH, trim_set, trim_set_sz,
>> +			       input_str, input_str_sz);
>> +	}
>> +}
>> +
>> +/**
>> + * Normalize args from @a argv input array when it has three args.
>> + *
>> + * Case: TRIM(LEADING/TRAILING/BOTH <character_set> FROM <str>)
>> + * If user has specified side keyword and <character_set>, then
>> + * call trimming procedure with that args.
>> + *
>> + * @param context SQL context.
>> + */
>> +static void
>> +trim_func_three_args(sql_context * context, int argc, sql_value **argv)
>> +{
>> +	const unsigned char *input_str;
>> +	assert(argc == 3);
>> +	assert(sql_value_type(argv[0]) == SQL_INTEGER);
>> +	(void) argc;
>> +
>> +	if ((input_str = sql_value_text(argv[2])) == NULL)
>> +		return;
>> +	int input_str_sz = sql_value_bytes(argv[2]);
>> +
>> +	const char unsigned *trim_set;
>> +	if ((trim_set = sql_value_text(argv[1])) != NULL) {
>> +		int trim_set_sz = sql_value_bytes(argv[1]);
>> +		trim_procedure(context, sql_value_int(argv[0]), trim_set,
>> +			       trim_set_sz, input_str, input_str_sz);
>> 	}
>> -	sql_result_text(context, (char *)zIn, nIn, SQL_TRANSIENT);
> 
> 15. Please, apply.
> 
> @@ -1448,21 +1446,18 @@ trim_func_two_args(sql_context * context, int argc, sql_value **argv)
> static void
> trim_func_three_args(sql_context * context, int argc, sql_value **argv)
> {
> -	const unsigned char *input_str;
> +	const unsigned char *input_str, *trim_set;
> 	assert(argc == 3);
> 	assert(sql_value_type(argv[0]) == SQL_INTEGER);
> 	(void) argc;
> 
> -	if ((input_str = sql_value_text(argv[2])) == NULL)
> +	if ((input_str = sql_value_text(argv[2])) == NULL ||
> +	    (trim_set = sql_value_text(argv[1])) == NULL)
> 		return;
> 	int input_str_sz = sql_value_bytes(argv[2]);
> -
> -	const char unsigned *trim_set;
> -	if ((trim_set = sql_value_text(argv[1])) != NULL) {
> -		int trim_set_sz = sql_value_bytes(argv[1]);
> -		trim_procedure(context, sql_value_int(argv[0]), trim_set,
> -			       trim_set_sz, input_str, input_str_sz);
> -	}
> +	int trim_set_sz = sql_value_bytes(argv[1]);
> +	trim_procedure(context, sql_value_int(argv[0]), trim_set, trim_set_sz,
> +		       input_str, input_str_sz);
> }
+static void
+trim_func_three_args(struct sql_context *context, int argc, sql_value **argv)
+{
+	assert(argc == 3);
+	(void) argc;
+
+	assert(sql_value_type(argv[0]) == SQL_INTEGER);
+	const unsigned char *input_str, *trim_set;
+	if ((input_str = sql_value_text(argv[2])) == NULL ||
+	    (trim_set = sql_value_text(argv[1])) == NULL)
+		return;
+
+	int trim_set_sz = sql_value_bytes(argv[1]);
+	int input_str_sz = sql_value_bytes(argv[2]);
+	trim_procedure(context, sql_value_int(argv[0]), trim_set, trim_set_sz,
+		       input_str, input_str_sz);
 }

>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index 099daf512..b49638d44 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> +
>> +%type expr_optional {struct Expr *}
>> +%destructor expr_optional {sql_expr_delete(pParse->db, $$, false);}
>> +
>> +expr_optional(A) ::= . { A = NULL; }
>> +expr_optional(A) ::= expr(X). { A = X.pExpr; }
>> +
>> +%type trim_specification {int}
> 
> 16. It is not int - it is enum trim_side_mask.
+%type trim_specification {enum trim_side_mask}

commit 9ae7a84bc59ded41302bde1dca0d0f82d540b960
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Thu Mar 28 14:01:33 2019 +0300

    sql: modify TRIM() function signature
    
    According to the ANSI standard, ltrim, rtrim and trim should
    be merged into one unified TRIM() function. The specialization of
    trimming (left, right or both and trimming characters) determined
    in arguments of this function.
    
    Closes #3879

diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index be7bd5545..76e3265e7 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -278,6 +278,10 @@ static Keyword aKeywordTable[] = {
   { "WHILE",                  "TK_STANDARD",    RESERVED,         true  },
   { "TEXT",                   "TK_TEXT",        RESERVED,         true  },
   { "TRUNCATE",               "TK_TRUNCATE",    ALWAYS,           true  },
+  { "TRIM",                   "TK_TRIM",        ALWAYS,           true  },
+  { "LEADING",                "TK_LEADING",     ALWAYS,           true  },
+  { "TRAILING",               "TK_TRAILING",    ALWAYS,           true  },
+  { "BOTH",                   "TK_BOTH",        ALWAYS,           true  },
 };
 
 /* Number of keywords */
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index abeecefa1..6f2a5e3f6 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1286,108 +1286,173 @@ replaceFunc(sql_context * context, int argc, sql_value ** argv)
 	sql_result_text(context, (char *)zOut, j, sql_free);
 }
 
-/*
- * Implementation of the TRIM(), LTRIM(), and RTRIM() functions.
- * The userdata is 0x1 for left trim, 0x2 for right trim, 0x3 for both.
+/**
+ * Remove characters included in @a trim_set from @a input_str
+ * until encounter a character that doesn't belong to @a trim_set.
+ * Remove from the side specified by @a flags.
+ * @param context SQL context.
+ * @param flags Trim specification: left, right or both.
+ * @param trim_set The set of characters for trimming.
+ * @param trim_set_sz Character set size in bytes.
+ * @param input_str Input string for trimming.
+ * @param input_str_sz Input string size in bytes.
  */
 static void
-trimFunc(sql_context * context, int argc, sql_value ** argv)
+trim_procedure(struct sql_context *context, enum trim_side_mask flags,
+	       const unsigned char *trim_set, int trim_set_sz,
+	       const unsigned char *input_str, int input_str_sz)
 {
-	const unsigned char *zIn;	/* Input string */
-	const unsigned char *zCharSet;	/* Set of characters to trim */
-	int nIn;		/* Number of bytes in input */
-	int flags;		/* 1: trimleft  2: trimright  3: trim */
-	int i;			/* Loop counter */
-	unsigned char *aLen = 0;	/* Length of each character in zCharSet */
-	unsigned char **azChar = 0;	/* Individual characters in zCharSet */
-	int nChar;		/* Number of characters in zCharSet */
-
-	if (sql_value_type(argv[0]) == SQL_NULL) {
+	const unsigned char *z = trim_set;
+	/*
+	 * Count the number of UTF-8 characters passing through
+	 * the entire char set, but not up to the '\0' or X'00'
+	 * character. This allows to handle trimming set
+	 * containing such characters.
+	 */
+	int char_cnt = sql_utf8_char_count(z, trim_set_sz);
+	if (char_cnt == 0)
+		goto result;
+	/* Individual characters in the character set. */
+	unsigned char **ind_chars =
+		contextMalloc(context,
+			      char_cnt * (sizeof(unsigned char *) + 1));
+	if (ind_chars == NULL)
 		return;
+	/* Length of each character in the character set. */
+	uint8_t *char_len = (uint8_t *)&ind_chars[char_cnt];
+	z = trim_set;
+	int i = 0;
+	char_cnt = 0;
+	int handled_bytes_cnt = trim_set_sz;
+	while(handled_bytes_cnt > 0) {
+		ind_chars[char_cnt] = (unsigned char *)(z + i);
+		SQL_UTF8_FWD_1(z, i, trim_set_sz);
+		char_len[char_cnt] = z + i - ind_chars[char_cnt];
+		handled_bytes_cnt -= char_len[char_cnt];
+		char_cnt++;
 	}
-	zIn = sql_value_text(argv[0]);
-	if (zIn == 0)
-		return;
-	nIn = sql_value_bytes(argv[0]);
-	assert(zIn == sql_value_text(argv[0]));
-	if (argc == 1) {
-		static const unsigned char lenOne[] = { 1 };
-		static unsigned char *const azOne[] = { (u8 *) " " };
-		nChar = 1;
-		aLen = (u8 *) lenOne;
-		azChar = (unsigned char **)azOne;
-		zCharSet = 0;
-	} else if ((zCharSet = sql_value_text(argv[1])) == 0) {
-		return;
-	} else {
-		const unsigned char *z = zCharSet;
-		int trim_set_sz = sql_value_bytes(argv[1]);
-		/*
-		* Count the number of UTF-8 characters passing
-		* through the entire char set, but not up
-		* to the '\0' or X'00' character. This allows
-		* to handle trimming set containing such
-		* characters.
-		*/
-		nChar = sql_utf8_char_count(z, trim_set_sz);
-		if (nChar > 0) {
-			azChar =
-			    contextMalloc(context,
-					  ((i64) nChar) * (sizeof(char *) + 1));
-			if (azChar == 0) {
-				return;
-			}
-			aLen = (unsigned char *)&azChar[nChar];
-			z = zCharSet;
-			i = 0;
-			nChar = 0;
-			int handled_bytes_cnt = trim_set_sz;
-			while(handled_bytes_cnt > 0) {
-				azChar[nChar] = (unsigned char *)(z + i);
-				SQL_UTF8_FWD_1(z, i, trim_set_sz);
-				aLen[nChar] = (u8) (z + i - azChar[nChar]);
-				handled_bytes_cnt -= aLen[nChar];
-				nChar++;
-			}
-		}
-	}
-	if (nChar > 0) {
-		flags = SQL_PTR_TO_INT(sql_user_data(context));
-		if (flags & 1) {
-			while (nIn > 0) {
-				int len = 0;
-				for (i = 0; i < nChar; i++) {
-					len = aLen[i];
-					if (len <= nIn
-					    && memcmp(zIn, azChar[i], len) == 0)
-						break;
-				}
-				if (i >= nChar)
+
+	if (char_cnt == 0)
+		goto result;
+	if ((flags & TRIM_LEADING) != 0) {
+		while (input_str_sz > 0) {
+			int len = 0;
+			for (i = 0; i < char_cnt; i++) {
+				len = char_len[i];
+				if (len <= input_str_sz
+				    && memcmp(input_str,
+					      ind_chars[i], len) == 0)
 					break;
-				zIn += len;
-				nIn -= len;
 			}
+			if (i >= char_cnt)
+				break;
+			input_str += len;
+			input_str_sz -= len;
 		}
-		if (flags & 2) {
-			while (nIn > 0) {
-				int len = 0;
-				for (i = 0; i < nChar; i++) {
-					len = aLen[i];
-					if (len <= nIn
-					    && memcmp(&zIn[nIn - len],
-						      azChar[i], len) == 0)
-						break;
-				}
-				if (i >= nChar)
+	}
+	if ((flags & TRIM_TRAILING) != 0) {
+		while (input_str_sz > 0) {
+			int len = 0;
+			for (i = 0; i < char_cnt; i++) {
+				len = char_len[i];
+				if (len <= input_str_sz
+				    && memcmp(&input_str[input_str_sz - len],
+					      ind_chars[i], len) == 0)
 					break;
-				nIn -= len;
 			}
-		}
-		if (zCharSet) {
-			sql_free(azChar);
+			if (i >= char_cnt)
+				break;
+			input_str_sz -= len;
 		}
 	}
-	sql_result_text(context, (char *)zIn, nIn, SQL_TRANSIENT);
+
+	if (trim_set_sz != 0)
+		sql_free(ind_chars);
+
+	result: sql_result_text(context, (char *)input_str, input_str_sz,
+				SQL_TRANSIENT);
+}
+
+/**
+ * Normalize args from @a argv input array when it has one arg
+ * only.
+ *
+ * Case: TRIM(<str>)
+ * Call trimming procedure with TRIM_BOTH as the flags and " " as
+ * the trimming set.
+ */
+static void
+trim_func_one_arg(struct sql_context *context, int argc, sql_value **argv)
+{
+	assert(argc == 1);
+	(void) argc;
+
+	const unsigned char *input_str;
+	if ((input_str = sql_value_text(argv[0])) == NULL)
+		return;
+
+	int input_str_sz = sql_value_bytes(argv[0]);
+	trim_procedure(context, TRIM_BOTH, (const unsigned char *) " ",
+		       1, input_str, input_str_sz);
+}
+
+/**
+ * Normalize args from @a argv input array when it has two args.
+ *
+ * Case: TRIM(<character_set> FROM <str>)
+ * If user has specified <character_set> only, call trimming
+ * procedure with TRIM_BOTH as the flags and that trimming set.
+ *
+ * Case: TRIM(LEADING/TRAILING/BOTH FROM <str>)
+ * If user has specified side keyword only, then call trimming
+ * procedure with the specified side and " " as the trimming set.
+ */
+static void
+trim_func_two_args(struct sql_context *context, int argc, sql_value **argv)
+{
+	assert(argc == 2);
+	(void) argc;
+
+	const unsigned char *input_str;
+	if ((input_str = sql_value_text(argv[1])) == NULL)
+		return;
+
+	int input_str_sz = sql_value_bytes(argv[1]);
+	const unsigned char *trim_set;
+	if (sql_value_type(argv[0]) == SQL_INTEGER) {
+		trim_procedure(context, sql_value_int(argv[0]),
+			       (const unsigned char *) " ", 1,
+			       input_str, input_str_sz);
+	} else if ((trim_set = sql_value_text(argv[0])) != NULL) {
+		int trim_set_sz = sql_value_bytes(argv[0]);
+		trim_procedure(context, TRIM_BOTH, trim_set, trim_set_sz,
+			       input_str, input_str_sz);
+	}
+}
+
+/**
+ * Normalize args from @a argv input array when it has three args.
+ *
+ * Case: TRIM(LEADING/TRAILING/BOTH <character_set> FROM <str>)
+ * If user has specified side keyword and <character_set>, then
+ * call trimming procedure with that args.
+ */
+static void
+trim_func_three_args(struct sql_context *context, int argc, sql_value **argv)
+{
+	assert(argc == 3);
+	(void) argc;
+
+	assert(sql_value_type(argv[0]) == SQL_INTEGER);
+	const unsigned char *input_str, *trim_set;
+	if ((input_str = sql_value_text(argv[2])) == NULL ||
+	    (trim_set = sql_value_text(argv[1])) == NULL)
+		return;
+
+	int trim_set_sz = sql_value_bytes(argv[1]);
+	int input_str_sz = sql_value_bytes(argv[2]);
+	trim_procedure(context, sql_value_int(argv[0]), trim_set, trim_set_sz,
+		       input_str, input_str_sz);
 }
 
 #ifdef SQL_ENABLE_UNKNOWN_SQL_FUNCTION
@@ -1818,12 +1883,9 @@ sqlRegisterBuiltinFunctions(void)
 			  FIELD_TYPE_INTEGER),
 		FUNCTION2(likely, 1, 0, 0, noopFunc, SQL_FUNC_UNLIKELY,
 			  FIELD_TYPE_INTEGER),
-		FUNCTION_COLL(ltrim, 1, 1, 0, trimFunc),
-		FUNCTION_COLL(ltrim, 2, 1, 0, trimFunc),
-		FUNCTION_COLL(rtrim, 1, 2, 0, trimFunc),
-		FUNCTION_COLL(rtrim, 2, 2, 0, trimFunc),
-		FUNCTION_COLL(trim, 1, 3, 0, trimFunc),
-		FUNCTION_COLL(trim, 2, 3, 0, trimFunc),
+		FUNCTION_COLL(trim, 1, 3, 0, trim_func_one_arg),
+		FUNCTION_COLL(trim, 2, 3, 0, trim_func_two_args),
+		FUNCTION_COLL(trim, 3, 3, 0, trim_func_three_args),
 		FUNCTION(min, -1, 0, 1, minmaxFunc, FIELD_TYPE_SCALAR),
 		FUNCTION(min, 0, 0, 1, 0, FIELD_TYPE_SCALAR),
 		AGGREGATE2(min, 1, 0, 1, minmaxStep, minMaxFinalize,
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 099daf512..a56ce7a10 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1032,6 +1032,55 @@ expr(A) ::= CAST(X) LP expr(E) AS typedef(T) RP(Y). {
   sqlExprAttachSubtrees(pParse->db, A.pExpr, E.pExpr, 0);
 }
 %endif  SQL_OMIT_CAST
+
+expr(A) ::= TRIM(X) LP trim_operands(Y) RP(E). {
+  A.pExpr = sqlExprFunction(pParse, Y, &X);
+  spanSet(&A, &X, &E);
+}
+
+%type trim_operands {struct ExprList *}
+%destructor trim_operands {sql_expr_list_delete(pParse->db, $$);}
+
+trim_operands(A) ::= trim_from_clause(F) expr(Y). {
+  A = sql_expr_list_append(pParse->db, F, Y.pExpr);
+}
+
+trim_operands(A) ::= expr(Y). {
+  A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
+}
+
+%type trim_from_clause {struct ExprList *}
+%destructor trim_from_clause {sql_expr_list_delete(pParse->db, $$);}
+
+/*
+ * The following two rules cover three cases of keyword
+ * (LEADING/TRAILING/BOTH) and <trim_character_set> combination.
+ * The case when both of them are absent is disallowed.
+ */
+trim_from_clause(A) ::= expr(Y) FROM. {
+  A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
+}
+
+trim_from_clause(A) ::= trim_specification(N) expr_optional(Y) FROM. {
+  struct Expr *p = sql_expr_new_dequoted(pParse->db, TK_INTEGER,
+                                         &sqlIntTokens[N]);
+  A = sql_expr_list_append(pParse->db, NULL, p);
+  if (Y != NULL)
+    A = sql_expr_list_append(pParse->db, A, Y);
+}
+
+%type expr_optional {struct Expr *}
+%destructor expr_optional {sql_expr_delete(pParse->db, $$, false);}
+
+expr_optional(A) ::= .        { A = NULL; }
+expr_optional(A) ::= expr(X). { A = X.pExpr; }
+
+%type trim_specification {enum trim_side_mask}
+
+trim_specification(A) ::= LEADING.  { A = TRIM_LEADING; }
+trim_specification(A) ::= TRAILING. { A = TRIM_TRAILING; }
+trim_specification(A) ::= BOTH.     { A = TRIM_BOTH; }
+
 expr(A) ::= id(X) LP distinct(D) exprlist(Y) RP(E). {
   if( Y && Y->nExpr>pParse->db->aLimit[SQL_LIMIT_FUNCTION_ARG] ){
     const char *err =
@@ -1294,7 +1343,7 @@ expr(A) ::= EXISTS(B) LP select(Y) RP(E). {
 }
 
 /* CASE expressions */
-expr(A) ::= CASE(C) case_operand(X) case_exprlist(Y) case_else(Z) END(E). {
+expr(A) ::= CASE(C) expr_optional(X) case_exprlist(Y) case_else(Z) END(E). {
   spanSet(&A,&C,&E);  /*A-overwrites-C*/
   A.pExpr = sqlPExpr(pParse, TK_CASE, X, 0);
   if( A.pExpr ){
@@ -1319,10 +1368,6 @@ case_exprlist(A) ::= WHEN expr(Y) THEN expr(Z). {
 %destructor case_else {sql_expr_delete(pParse->db, $$, false);}
 case_else(A) ::=  ELSE expr(X).         {A = X.pExpr;}
 case_else(A) ::=  .                     {A = 0;} 
-%type case_operand {Expr*}
-%destructor case_operand {sql_expr_delete(pParse->db, $$, false);}
-case_operand(A) ::= expr(X).            {A = X.pExpr; /*A-overwrites-X*/} 
-case_operand(A) ::= .                   {A = 0;} 
 
 %type exprlist {ExprList*}
 %destructor exprlist {sql_expr_list_delete(pParse->db, $$);}
diff --git a/src/box/sql/parse_def.c b/src/box/sql/parse_def.c
index 49c76a326..aa1323cb2 100644
--- a/src/box/sql/parse_def.c
+++ b/src/box/sql/parse_def.c
@@ -34,7 +34,9 @@
 
 const struct Token sqlIntTokens[] = {
 	{"0", 1, false},
-	{"1", 1, false}
+	{"1", 1, false},
+	{"2", 1, false},
+	{"3", 1, false},
 };
 
 void
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index a1af2bacd..5899a7e4e 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -87,7 +87,7 @@ struct Token {
 	bool isReserved;
 };
 
-/** Constant tokens for values 0 and 1. */
+/** Constant tokens for integer values. */
 extern const struct Token sqlIntTokens[];
 
 /** Generate a Token object from a string. */
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index b322602dc..d5a3e15c1 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -1680,6 +1680,17 @@ struct FuncDestructor {
 					 * single query - might change over time
 					 */
 
+/*
+ * Trim side mask components. TRIM_LEADING means to trim left side
+ * only. TRIM_TRAILING is to trim right side only. TRIM_BOTH is to
+ * trim both sides.
+ */
+enum trim_side_mask {
+	TRIM_LEADING = 1,
+	TRIM_TRAILING = 2,
+	TRIM_BOTH = TRIM_LEADING | TRIM_TRAILING
+};
+
 /*
  * The following three macros, FUNCTION(), LIKEFUNC() and AGGREGATE() are
  * used to create the initializers for the FuncDef structures.
diff --git a/test/sql-tap/badutf1.test.lua b/test/sql-tap/badutf1.test.lua
index d104efaa9..9079dfe25 100755
--- a/test/sql-tap/badutf1.test.lua
+++ b/test/sql-tap/badutf1.test.lua
@@ -302,7 +302,7 @@ test:do_test(
 test:do_test(
     "badutf-4.1",
     function()
-        return test:execsql2("SELECT hex(trim('\x80\x80\x80\xf0\x80\x80\x80\xff','\x80\xff')) AS x")
+        return test:execsql2("SELECT hex(TRIM('\x80\xff' FROM '\x80\x80\x80\xf0\x80\x80\x80\xff')) AS x")
     end, {
         -- <badutf-4.1>
         "X", "F0"
@@ -312,7 +312,7 @@ test:do_test(
 test:do_test(
     "badutf-4.2",
     function()
-        return test:execsql2("SELECT hex(ltrim('\x80\x80\x80\xf0\x80\x80\x80\xff','\x80\xff')) AS x")
+        return test:execsql2("SELECT hex(TRIM(LEADING '\x80\xff' FROM '\x80\x80\x80\xf0\x80\x80\x80\xff')) AS x")
     end, {
         -- <badutf-4.2>
         "X", "F0808080FF"
@@ -322,7 +322,7 @@ test:do_test(
 test:do_test(
     "badutf-4.3",
     function()
-        return test:execsql2("SELECT hex(rtrim('\x80\x80\x80\xf0\x80\x80\x80\xff','\x80\xff')) AS x")
+        return test:execsql2("SELECT hex(TRIM(TRAILING '\x80\xff' FROM '\x80\x80\x80\xf0\x80\x80\x80\xff')) AS x")
     end, {
         -- <badutf-4.3>
         "X", "808080F0"
@@ -332,7 +332,7 @@ test:do_test(
 test:do_test(
     "badutf-4.4",
     function()
-        return test:execsql2("SELECT hex(trim('\x80\x80\x80\xf0\x80\x80\x80\xff','\xff\x80')) AS x")
+        return test:execsql2("SELECT hex(TRIM('\xff\x80' FROM '\x80\x80\x80\xf0\x80\x80\x80\xff')) AS x")
     end, {
         -- <badutf-4.4>
         "X", "808080F0808080FF"
@@ -342,7 +342,7 @@ test:do_test(
 test:do_test(
     "badutf-4.5",
     function()
-        return test:execsql2("SELECT hex(trim('\xff\x80\x80\xf0\x80\x80\x80\xff','\xff\x80')) AS x")
+        return test:execsql2("SELECT hex(TRIM('\xff\x80' FROM '\xff\x80\x80\xf0\x80\x80\x80\xff')) AS x")
     end, {
         -- <badutf-4.5>
         "X", "80F0808080FF"
@@ -352,7 +352,7 @@ test:do_test(
 test:do_test(
     "badutf-4.6",
     function()
-        return test:execsql2("SELECT hex(trim('\xff\x80\xf0\x80\x80\x80\xff','\xff\x80')) AS x")
+        return test:execsql2("SELECT hex(TRIM('\xff\x80' FROM '\xff\x80\xf0\x80\x80\x80\xff')) AS x")
     end, {
         -- <badutf-4.6>
         "X", "F0808080FF"
@@ -362,7 +362,7 @@ test:do_test(
 test:do_test(
     "badutf-4.7",
     function()
-        return test:execsql2("SELECT hex(trim('\xff\x80\xf0\x80\x80\x80\xff','\xff\x80\x80')) AS x")
+        return test:execsql2("SELECT hex(TRIM('\xff\x80\x80' FROM '\xff\x80\xf0\x80\x80\x80\xff')) AS x")
     end, {
         -- <badutf-4.7>
         "X", "FF80F0808080FF"
diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index 251cc3534..fe9a98191 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(14586)
+test:plan(14590)
 
 --!./tcltestrunner.lua
 -- 2001 September 15
@@ -1915,7 +1915,7 @@ test:do_catchsql_test(
         SELECT trim(1,2,3)
     ]], {
         -- <func-22.1>
-        1, "wrong number of arguments to function TRIM()"
+        1, "Syntax error near ','"
         -- </func-22.1>
     })
 
@@ -1925,7 +1925,7 @@ test:do_catchsql_test(
         SELECT ltrim(1,2,3)
     ]], {
         -- <func-22.2>
-        1, "wrong number of arguments to function LTRIM()"
+        1, "Function 'LTRIM' does not exist"
         -- </func-22.2>
     })
 
@@ -1935,7 +1935,7 @@ test:do_catchsql_test(
         SELECT rtrim(1,2,3)
     ]], {
         -- <func-22.3>
-        1, "wrong number of arguments to function RTRIM()"
+        1, "Function 'RTRIM' does not exist"
         -- </func-22.3>
     })
 
@@ -1952,7 +1952,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.5",
     [[
-        SELECT ltrim('  hi  ');
+        SELECT TRIM(LEADING FROM '  hi  ');
     ]], {
         -- <func-22.5>
         "hi  "
@@ -1962,7 +1962,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.6",
     [[
-        SELECT rtrim('  hi  ');
+        SELECT TRIM(TRAILING FROM '  hi  ');
     ]], {
         -- <func-22.6>
         "  hi"
@@ -1972,7 +1972,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.7",
     [[
-        SELECT trim('  hi  ','xyz');
+        SELECT TRIM('xyz' FROM '  hi  ');
     ]], {
         -- <func-22.7>
         "  hi  "
@@ -1982,7 +1982,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.8",
     [[
-        SELECT ltrim('  hi  ','xyz');
+        SELECT TRIM(LEADING 'xyz' FROM '  hi  ');
     ]], {
         -- <func-22.8>
         "  hi  "
@@ -1992,7 +1992,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.9",
     [[
-        SELECT rtrim('  hi  ','xyz');
+        SELECT TRIM(TRAILING 'xyz' FROM '  hi  ');
     ]], {
         -- <func-22.9>
         "  hi  "
@@ -2002,7 +2002,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.10",
     [[
-        SELECT trim('xyxzy  hi  zzzy','xyz');
+        SELECT TRIM('xyz' FROM 'xyxzy  hi  zzzy');
     ]], {
         -- <func-22.10>
         "  hi  "
@@ -2012,7 +2012,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.11",
     [[
-        SELECT ltrim('xyxzy  hi  zzzy','xyz');
+        SELECT TRIM(LEADING 'xyz' FROM 'xyxzy  hi  zzzy');
     ]], {
         -- <func-22.11>
         "  hi  zzzy"
@@ -2022,7 +2022,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.12",
     [[
-        SELECT rtrim('xyxzy  hi  zzzy','xyz');
+        SELECT TRIM(TRAILING 'xyz' FROM 'xyxzy  hi  zzzy');
     ]], {
         -- <func-22.12>
         "xyxzy  hi  "
@@ -2032,7 +2032,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.13",
     [[
-        SELECT trim('  hi  ','');
+        SELECT TRIM('' FROM '  hi  ');
     ]], {
         -- <func-22.13>
         "  hi  "
@@ -2043,7 +2043,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.14",
     [[
-        SELECT hex(trim(x'c280e1bfbff48fbfbf6869',x'6162e1bfbfc280'))
+        SELECT hex(TRIM(x'6162e1bfbfc280' FROM x'c280e1bfbff48fbfbf6869'))
     ]], {
         -- <func-22.14>
         "F48FBFBF6869"
@@ -2052,8 +2052,8 @@ test:do_execsql_test(
 
 test:do_execsql_test(
     "func-22.15",
-    [[SELECT hex(trim(x'6869c280e1bfbff48fbfbf61',
-                         x'6162e1bfbfc280f48fbfbf'))]], {
+    [[SELECT hex(TRIM(x'6162e1bfbfc280f48fbfbf'
+                      FROM x'6869c280e1bfbff48fbfbf61'))]], {
         -- <func-22.15>
         "6869"
         -- </func-22.15>
@@ -2062,7 +2062,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.16",
     [[
-        SELECT hex(trim(x'ceb1ceb2ceb3',x'ceb1'));
+        SELECT hex(TRIM(x'ceb1' FROM x'ceb1ceb2ceb3'));
     ]], {
         -- <func-22.16>
         "CEB2CEB3"
@@ -2083,7 +2083,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.21",
     [[
-        SELECT typeof(trim(NULL,'xyz'));
+        SELECT typeof(TRIM('xyz' FROM NULL));
     ]], {
         -- <func-22.21>
         "null"
@@ -2093,7 +2093,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.22",
     [[
-        SELECT typeof(trim('hello',NULL));
+        SELECT typeof(TRIM(NULL FROM 'hello'));
     ]], {
         -- <func-22.22>
         "null"
@@ -2105,7 +2105,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.23",
     [[
-        SELECT TRIM(X'004100', X'00');
+        SELECT TRIM(X'00' FROM X'004100');
     ]], {
         -- <func-22.23>
         "A"
@@ -2115,7 +2115,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.24",
     [[
-        SELECT TRIM(X'004100', X'0000');
+        SELECT TRIM(X'0000' FROM X'004100');
     ]], {
         -- <func-22.24>
         "A"
@@ -2125,7 +2125,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.25",
     [[
-        SELECT TRIM(X'004100', X'0042');
+        SELECT TRIM(X'0042' FROM X'004100');
     ]], {
         -- <func-22.25>
         "A"
@@ -2135,7 +2135,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.26",
     [[
-        SELECT TRIM(X'00004100420000', X'00');
+        SELECT TRIM(X'00' FROM X'00004100420000');
     ]], {
         -- <func-22.26>
         "A\0B"
@@ -2145,7 +2145,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.27",
     [[
-        SELECT LTRIM(X'004100', X'00');
+        SELECT TRIM(LEADING X'00' FROM X'004100');
     ]], {
         -- <func-22.27>
         "A\0"
@@ -2155,7 +2155,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.28",
     [[
-        SELECT LTRIM(X'004100', X'0000');
+        SELECT TRIM(LEADING X'0000' FROM X'004100');
     ]], {
         -- <func-22.28>
         "A\0"
@@ -2165,7 +2165,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.29",
     [[
-        SELECT LTRIM(X'004100', X'0042');
+        SELECT TRIM(LEADING X'0042' FROM X'004100');
     ]], {
         -- <func-22.29>
         "A\0"
@@ -2175,7 +2175,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.30",
     [[
-        SELECT LTRIM(X'00004100420000', X'00');
+        SELECT TRIM(LEADING X'00' FROM X'00004100420000');
     ]], {
         -- <func-22.30>
         "A\0B\0\0"
@@ -2185,7 +2185,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.31",
     [[
-        SELECT RTRIM(X'004100', X'00');
+        SELECT TRIM(TRAILING X'00' FROM X'004100');
     ]], {
         -- <func-22.31>
         "\0A"
@@ -2195,7 +2195,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.32",
     [[
-        SELECT RTRIM(X'004100', X'0000');
+        SELECT TRIM(TRAILING X'0000' FROM X'004100');
     ]], {
         -- <func-22.32>
         "\0A"
@@ -2205,7 +2205,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.33",
     [[
-        SELECT RTRIM(X'004100', X'0042');
+        SELECT TRIM(TRAILING X'0042' FROM X'004100');
     ]], {
         -- <func-22.33>
         "\0A"
@@ -2215,13 +2215,56 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.34",
     [[
-        SELECT RTRIM(X'00004100420000', X'00');
+        SELECT TRIM(TRAILING X'00' FROM X'00004100420000');
     ]], {
         -- <func-22.34>
         "\0\0A\0B"
         -- </func-22.34>
     })
 
+-- gh-3879 Check new TRIM() grammar, particularly BOTH keyword and
+-- FROM without any agrs before. LEADING and TRAILING keywords is
+-- checked above.
+
+test:do_execsql_test(
+    "func-22.35",
+    [[
+        SELECT TRIM(BOTH FROM '  hi  ');
+    ]], {
+        -- <func-22.35>
+        "hi"
+        -- </func-22.35>
+    })
+test:do_execsql_test(
+    "func-22.36",
+    [[
+        SELECT TRIM(BOTH 'xyz' FROM '  hi  ');
+    ]], {
+        -- <func-22.36>
+        "  hi  "
+        -- </func-22.36>
+    })
+
+test:do_execsql_test(
+    "func-22.37",
+    [[
+        SELECT TRIM(BOTH 'xyz' FROM 'xyxzy  hi  zzzy');
+    ]], {
+        -- <func-22.37>
+        "  hi  "
+        -- </func-22.37>
+    })
+
+test:do_catchsql_test(
+    "func-22.38",
+    [[
+        SELECT TRIM(FROM 'xyxzy');
+    ]], {
+        -- <func-22.38>
+        1, "Syntax error near 'FROM'"
+        -- </func-22.38>
+    })
+
 -- This is to test the deprecated sql_aggregate_count() API.
 --
 --test:do_test(
@@ -2838,16 +2881,16 @@ test:do_execsql_test(
     "SELECT TRIM(CHAR(32,00,32,00,32));",
     {string.char(00,32,00)})
 
--- LTRIM
+-- LEFT TRIM
 test:do_execsql_test(
     "func-70",
-    "SELECT LTRIM(CHAR(32,00,32,00,32));",
+    "SELECT TRIM(LEADING FROM CHAR(32,00,32,00,32));",
     {string.char(00,32,00,32)})
 
--- RTRIM
+-- RIGHT TRIM
 test:do_execsql_test(
     "func-71",
-    "SELECT RTRIM(CHAR(32,00,32,00,32));",
+    "SELECT TRIM(TRAILING FROM CHAR(32,00,32,00,32));",
     {string.char(32,00,32,00)})
 
 -- GROUP_CONCAT
diff --git a/test/sql-tap/with1.test.lua b/test/sql-tap/with1.test.lua
index 495aa4ee4..ec45e5e76 100755
--- a/test/sql-tap/with1.test.lua
+++ b/test/sql-tap/with1.test.lua
@@ -550,7 +550,7 @@ test:do_execsql_test("8.1-mandelbrot", [[
       SELECT group_concat( substr(' .+*#', 1+min(iter/7,4), 1), '') 
       FROM m2 GROUP BY cy
     )
-  SELECT group_concat(rtrim(t),x'0a') FROM a;
+  SELECT group_concat(TRIM(TRAILING FROM t),x'0a') FROM a;
 ]], {
   -- <8.1-mandelbrot>
   [[                                    ....#