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

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

Hi! Thanks for the review.

>>> 8. Please, create a enum with normal names for these constants.
>> +enum trim_specification {
>> +	LEADING = 1,
>> +	TRAILING = 2,
>> +	BOTH = 3
> 
> 1. These values are used as a bitmask in the TRIM function
> implementation. I expected that you would account it. BOTH
> should be a bit combination of LEADING and TRAILING.
> 
> Also, in such a case it should be 'trim_side_mask' enum,
> not just 'trim_specification' - what does it specify.
> 
> In addition, we have a strict policy of naming enum values,
> because they are visible in the whole namespace. We do not
> have C++ namespaces. C-way of namespacing is prefixing all
> functions and constants with a certain name. It means, that
> the values should be prefixed with uppercased enum name
> (or its part, when it is too long). Here I would use just
> 'TRIM_' prefix.
> 
> Finally, add a comment to that enum.
I accounted that, but I didn’t think that it should be implemented that way.

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
+};

> 2. Ok, you added enum, but you do not use it at all
> anywhere. What is a point of such enum? You still use
> constants in both parse.y and trim_procedure.
> 
> Please, do a self-review. In is easy to find such
> places by yourself just diligently scanning the diff
> couple of times before a send.
+trim_specification(A) ::= LEADING.  {A = TRIM_LEADING;}
+trim_specification(A) ::= TRAILING. {A = TRIM_TRAILING;}
+trim_specification(A) ::= BOTH.     {A = TRIM_BOTH;}

-		flags = SQL_PTR_TO_INT(sql_user_data(context));
-		if (flags & 1) {
-			while (nIn > 0) {
+	if (char_cnt > 0) {
+		if ((flags & TRIM_LEADING) != 0) {


-		if (flags & 2) {
-			while (nIn > 0) {
+		if ((flags & TRIM_TRAILING) != 0) {

>>> 14. Better write three trim functions taking different number of
>>> args, converting them to normal types, and calling the single
>>> trim function. Instead of making a pile of 'if's about argc inside
>>> the current implementation.
>> Done. But now I have dublicated pieces of code:
> 
> 4. Then do not duplicate and extract it into another function. It is
> one of your tasks as a programmer to reduce code duplication. You should
> not be a silent text-editor into which I insert my own code and ideas
> via the mailing list.
> 
> Probably after fixing my next comments the code duplication will be
> minor or will even disappear.
Yes. Now six lines is duplicated only. I don’t think that it requires a seperate
function.

+	const unsigned char *input_str;
+	assert(argc == 1);
+	(void) argc;
+
+	if ((input_str = sql_value_text(argv[0])) == NULL)
+		return;
+	int input_str_sz = sql_value_bytes(argv[0]);

> 
>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
>> index abeecefa1..bf7e7a652 100644
>> --- a/src/box/sql/func.c
>> +++ b/src/box/sql/func.c
>> @@ -1286,108 +1286,223 @@ 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.
>> +enum trim_specification {
>> +	LEADING = 1,
>> +	TRAILING = 2,
>> +	BOTH = 3
>> +};
>> +
>> +/**
>> + * Remove chars included into @a collation from @a input_str.
>> + * @param context SQL context.
>> + * @param flags Trim specification: left, right or both.
>> + * @param collation Character set.
>> + * @param coll_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(sql_context * context, enum trim_specification flags,
>> +	     const unsigned char *collation, int coll_sz,
>> +	     const unsigned char *input_str, int input_str_sz)
> 
> 5. Broken alignment.
Fixed.
+trim_procedure(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)

> 6. Why do you really need 'unsigned char'? I do not see any
> arithmetical operations here. Only assignments.
> 
>> {
>> -	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;
>> +	/* 
I need it, because of "sql_utf8_char_count(const unsigned char *str, int byte_len);” or
"const unsigned char *sql_value_text(sql_value *);"

> 7. Trailing whitespaces here and below. As I know, git highlights them
> with red color, which means, that you haven't reviewed that patch
> before sending. Please, do it next time. Also, you can avoid automatic
> trailing whitespaces if install one of the comment packages for Sublime.
> 
>> +	 * Length of each character in collation.
Removed.

> 8. Ok, now I see what did you mean as 'character set' in the
> previous version. Sorry, in such a case it is not collation of
> course, and it is strange, that you blindly renamed it without any
> opposition. It is ok to argue with me.
Now I name it “trim_set”.

+trim_procedure(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)
>> +	 */
>> +	unsigned char *aLen = 0;
> 
> 9. Please, do not use camel code style for new code. We
> never use it in Tarantool. Use normal names.
+	/*
+	 * Length of each character in the character set.
+	 */
+	char unsigned *char_len = 0;
+	/*
+	 * Individual characters in the character set.
+	 */
+	char unsigned **ind_chars = 0;
+	/*
+	 * Number of characters in the character set.
+	 */
+	int char_cnt;

>> +	/*
>> +	 * Individual characters in collation.
>> +	 */
>> +	unsigned char **azChar = 0;
>> +	/* 
>> +	 * Number of characters in collation.
>> +	 */
>> +	int nChar;
>> 
>> +	const unsigned char *z = collation;
>> +	/*
>> +	 * 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.
> 
> 10. The comment's indentation is reduced and the text can be
> realligned with less number of lines.
+	int i;
+	/* Length of each character in the character set. */
+	char unsigned *char_len = 0;
+	/* Individual characters in the character set. */
+	char unsigned **ind_chars = 0;
+	/* Number of characters in the character set. */
+	int char_cnt;
+
+	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.
+	 */

>> +	 */
>> +	nChar = sql_utf8_char_count(z, coll_sz);
> 
> 11. It is not C89. You do not need to declare all the variables
> at the beginning of function before their usage.

+	int char_cnt = sql_utf8_char_count(z, trim_set_sz);

>> + * 
>> + * Case: TRIM(<str>)
>> + * Call trimming procedure with BOTH as the flags and " " as the collation.
>> + *
>> + * @param context SQL context.
>> + * @param argc Number of args.
>> + * @param argv Args array.
> 
> 13. Comments on such simple args are useless and on the other hand there
> is nothing more to say. We often omit @param/@retval section in such a
> case, and I think it is applicable here.
> 
> I mean, that everything above first @param is ok, but below is not
> necessary. You can keep it if you want, up to you.
+/**
+ * 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.
+ */
+static void
+trim_func_one_arg(sql_context * context, int argc, sql_value **argv)

>> + */
>> +static void
>> +trim_func_one_arg(sql_context * context, int argc, sql_value **argv)
>> +{
>> +	const unsigned char *input_str;
>> +	assert(argc == 1);
>> +	(void) argc;
>> +
>> +	if (sql_value_type(argv[0]) == SQL_NULL) {
>> +		return;
>> +	}
> 
> 14. We do not use curly braces when 'if' or 'for' body
> consists of one line. What is more, you do not need
> this check at all, because sql_value_text returns NULL,
> when value is NULL as well. The same in other helper
> functions.
-	if (sql_value_type(argv[0]) == SQL_NULL) {
-		return;

>> +	if ((input_str = sql_value_text(argv[0])) == NULL) {
>> +		return;
>> +	}> +
>> +	int input_str_sz = sql_value_bytes(argv[0]);
>> +	assert(input_str == sql_value_text(argv[0]));
> 
> 15. What is a point of that assertion? You assigned input_str
> to this value literally 5 lines above.
-	assert(zIn == sql_value_text(argv[0]));

>> +
>> +	trim_procedure(context, BOTH, (const unsigned char *) " ",
>> +		       1, input_str, input_str_sz);
>> +}
>> +
>> +/**
>> + * Normalize args from @a argv input array when it has two args.
>> + * 
>> + * Case: TRIM(<trim_collation> FROM <str>)
>> + * If user has specified <trim_collation> only, call trimming procedure with
>> + * BOTH as the flags and that collation.
>> + *
>> + * Case: TRIM(LEADING/TRAILING/BOTH FROM <str>)
>> + * If user has specified side keyword only, call trimming procedure
>> + * with the specified side and " " as the collation.
>> + *
>> + * @param context SQL context.
>> + * @param argc Number of args.
>> + * @param argv Args array.
>> + */
>> +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 (sql_value_type(argv[1]) == SQL_NULL) {
>> +		return;
>> +	}
>> +	if ((input_str = sql_value_text(argv[1])) == NULL) {
>> +		return;
>> +	}
>> +
>> +	int input_str_sz = sql_value_bytes(argv[1]);
>> +	assert(input_str == sql_value_text(argv[1]));
>> +
>> +	const unsigned char *collation;
>> +	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 ((collation = sql_value_text(argv[0])) == NULL) {
>> +		return;
>> +	} else {
>> +		int coll_sz = sql_value_bytes(argv[0]);
>> +		trim_procedure(context, BOTH, collation, coll_sz, input_str,
>> +			       input_str_sz);
>> +	}
>> +}
>> +
>> +/**
>> + * Normalize args from @a argv input array when it has three args.
>> + *
>> + * Case: TRIM(LEADING/TRAILING/BOTH <trim_collation> FROM <str>)
>> + * User has specified side keyword and <trim_collation>, call trimming
>> + * procedure with that args.
>> + *
>> + * @param context SQL context.
>> + * @param argc Number of args.
>> + * @param argv Args array.
>> + */
>> +static void
>> +trim_func_three_args(sql_context * context, int argc, sql_value **argv)
>> +{
>> +	const unsigned char *input_str;
>> +	assert(argc == 3);
>> +	(void) argc;
>> +
>> +	if (sql_value_type(argv[2]) == SQL_NULL) {
>> +		return;
>> +	}
>> +	if ((input_str = sql_value_text(argv[2])) == NULL) {
>> +		return;
>> +	}
>> +
>> +	int input_str_sz = sql_value_bytes(argv[2]);
>> +	assert(input_str == sql_value_text(argv[2]));
>> +
>> +	const unsigned char *collation;
>> +	assert(sql_value_type(argv[0]) == SQL_INTEGER);
>> +	if ((collation = sql_value_text(argv[1])) != 0) {
> 
> 16. As I said in the previous review, and in reviews to
> other patches - use NULL to check if a pointer is NULL.
> 
> When a code hunk is tall, and someone sees code like
> 
>    variable = func()
>    if (variable != 0)
>        ....
> 
> they could think that the variable is integer. It is
> confusing (variable can be declared somewhere above
> and the one does not see its type).
+	const char unsigned *trim_set;
+	if ((trim_set = sql_value_text(argv[1])) != NULL) {

>> +	}
>> }
>> 
>> #ifdef SQL_ENABLE_UNKNOWN_SQL_FUNCTION
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index 099daf512..985d33605 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -1032,6 +1032,51 @@ 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, $$); }
>> +
>> +trim_from_clause(A) ::= expr(Y) FROM. {
>> +    A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
>> +}
>> +
>> +trim_from_clause(A) ::= trim_specification(N) trim_character(Y) FROM. {
> 
> 18. I understand, why you did not use trim_character rule above,
> but someone looking at this code first time and not seen our
> discussion will not understand. I would add a comment about it.
+/*
+ * 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.
+ */

>> +  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 trim_character {struct Expr *}
>> +%destructor trim_character {sql_expr_delete(pParse->db, $$, false);}
>> +
>> +trim_character(A) ::= . { A = NULL; }
>> +trim_character(A) ::= expr(X). { A = X.pExpr; }
> 
> 19. Exactly the same rule already exists: case_operand. I think, it
> is worth merging them into one rule like
> 
>    expr_optional(A) ::= . { A = NULL; }
>    expr_optional(A) ::= expr(X). { A = X.pExpr; }
> 
> And using in both places.
+%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 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 trim_specification {int}
>> +
>> +trim_specification(A) ::= LEADING.  {A = 1;}
>> +trim_specification(A) ::= TRAILING. {A = 2;}
>> +trim_specification(A) ::= BOTH.     {A = 3;}
>> +
>> 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.

>>     ]], {
>>         -- <func-22.1>
>> -        1, "wrong number of arguments to function TRIM()"
>> +        1, "Syntax error near ','"
>>         -- </func-22.1>
>>     })
>> @@ -2215,13 +2215,55 @@ 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.
> 
> 21. Out of 66.
+-- gh-3879 Check new TRIM() grammar, particularly BOTH keyword and
+-- FROM without any agrs before. LEADING and TRAILING keywords is
+-- checked above.

commit 37d84a94298da72f344c6d503cb794fe8dd1e9d7
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..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.
+ * @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(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 */
+	int i;
+	/* Length of each character in the character set. */
+	char unsigned *char_len = 0;
+	/* Individual characters in the character set. */
+	char unsigned **ind_chars = 0;
 
-	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) *
+				  (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]);
+			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) {
+		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)
 						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.
+ */
+static void
+trim_func_one_arg(sql_context * context, int argc, sql_value **argv)
+{
+	const unsigned char *input_str;
+	assert(argc == 1);
+	(void) argc;
+
+	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.
+ *
+ * @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 {
+		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);
 }
 
 #ifdef SQL_ENABLE_UNKNOWN_SQL_FUNCTION
@@ -1818,12 +1893,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..b49638d44 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1032,6 +1032,56 @@ 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 {int}
+
+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 +1344,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 +1369,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..d32bafae0 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..165eafb6d 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)
     ]], {
         -- <func-22.1>
-        1, "wrong number of arguments to function TRIM()"
+        1, "Syntax error near ','"
         -- </func-22.1>
     })
 
 test:do_catchsql_test(
     "func-22.2",
     [[
-        SELECT ltrim(1,2,3)
+        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>
     })
 
 test:do_catchsql_test(
     "func-22.3",
     [[
-        SELECT rtrim(1,2,3)
+        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>
     })
 
 test:do_execsql_test(
     "func-22.4",
     [[
-        SELECT trim('  hi  ');
+        SELECT TRIM('  hi  ');
     ]], {
         -- <func-22.4>
         "hi"
@@ -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"
@@ -2073,7 +2073,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-22.20",
     [[
-        SELECT typeof(trim(NULL));
+        SELECT typeof(TRIM(NULL));
     ]], {
         -- <func-22.20>
         "null"
@@ -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..19953e434 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>
   [[                                    ....#