[tarantool-patches] [PATCH] sql: make GREATEST/LEAST built-ins accept at least two args

[tarantool-patches] [PATCH] sql: make GREATEST/LEAST built-ins accept at least two args

[Nikita Pettik]

Before a46b52004 SQL implementation featured min()/max() functions
overloading: if one argument was passed, then aggregate version would be
invoked; otherwise - scalar one. We decided to get rid of it and rename
scalar version to LEAST()/GREATEST() correspondingly. However, assertion
inside their implementations has been remained: it verifies that number
of passed arguments is greater than 1. On the other hand, now one can
pass literally any number of arguments to this function, including one
(which results in fired mentioned assertion) and zero (which leads to
NULL dereference in expr.c: these functions are marked with
SQL_FUNC_NEEDCOLL flag, and as a consequence they are assumed to have at
least one argument). Firstly, let's place check that number of passed
arguments more than one. Secondly, let's not assume that functions with
SQL_FUNC_NEEDCOLL must have any arguments.

Closes #4453
---
Branch: https://github.com/tarantool/tarantool/tree/np/gh-4453-remove-redundant-assertion
Issue: https://github.com/tarantool/tarantool/issues/4453

 src/box/errcode.h           |  1 +
 src/box/sql/expr.c          |  3 ++-
 src/box/sql/func.c          |  7 ++++++-
 test/box/misc.result        |  1 +
 test/sql-tap/func5.test.lua | 23 ++++++++++++++++++++++-
 5 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/src/box/errcode.h b/src/box/errcode.h
index 46b0b365a..484097749 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -253,6 +253,7 @@ struct errcode_record {
 	/*198 */_(ER_FUNC_INDEX_FUNC,		"Failed to build a key for functional index '%s' of space '%s': %s") \
 	/*199 */_(ER_FUNC_INDEX_FORMAT,		"Key format doesn't match one defined in functional index '%s' of space '%s': %s") \
 	/*200 */_(ER_FUNC_INDEX_PARTS,		"Wrong functional index definition: %s") \
+	/*201 */_(ER_FUNC_WRONG_ARG_COUNT,      "Wrong number of arguments is passed to %s(): expected %s, got %d") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 1f9d91705..1957bd4c8 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4049,7 +4049,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			 * is done using ANSI rules from
 			 * collations_check_compatibility().
 			 */
-			if ((pDef->funcFlags & SQL_FUNC_NEEDCOLL) != 0) {
+			if ((pDef->funcFlags & SQL_FUNC_NEEDCOLL) != 0 &&
+			    nFarg > 0) {
 				struct coll *unused = NULL;
 				uint32_t curr_id = COLL_NONE;
 				bool is_curr_forced = false;
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 07c019db9..0c28cec29 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -81,8 +81,13 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
 	int iBest;
 	struct coll *pColl;
 
-	assert(argc > 1);
 	mask = sql_user_data(context) == 0 ? 0 : -1;
+	if (argc < 2) {
+		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT,
+		mask ? "GREATEST" : "LEAST", "at least two", argc);
+		context->is_aborted = true;
+		return;
+	}
 	pColl = sqlGetFuncCollSeq(context);
 	assert(mask == -1 || mask == 0);
 	iBest = 0;
diff --git a/test/box/misc.result b/test/box/misc.result
index 287d84e5b..e4de2e9b3 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -530,6 +530,7 @@ t;
   198: box.error.FUNC_INDEX_FUNC
   199: box.error.FUNC_INDEX_FORMAT
   200: box.error.FUNC_INDEX_PARTS
+  201: box.error.FUNC_WRONG_ARG_COUNT
 ...
 test_run:cmd("setopt delimiter ''");
 ---
diff --git a/test/sql-tap/func5.test.lua b/test/sql-tap/func5.test.lua
index 0b255e659..e84ca25be 100755
--- a/test/sql-tap/func5.test.lua
+++ b/test/sql-tap/func5.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(19)
+test:plan(22)
 
 --!./tcltestrunner.lua
 -- 2010 August 27
@@ -257,4 +257,25 @@ test:do_execsql_test(
         SELECT LEAST(false, 'STR', 1, 0.5);
     ]], { false } )
 
+-- gh-4453: GREATEST()/LEAST() require at least two arguments
+-- be passed to these functions.
+--
+test:do_catchsql_test(
+    "func-5-5.1",
+    [[
+        SELECT LEAST(false);
+    ]], { 1, "Wrong number of arguments is passed to LEAST(): expected at least two, got 1" } )
+
+test:do_catchsql_test(
+    "func-5-5.2",
+    [[
+        SELECT GREATEST('abc');
+    ]], { 1, "Wrong number of arguments is passed to GREATEST(): expected at least two, got 1" } )
+
+test:do_catchsql_test(
+    "func-5-5.3",
+    [[
+        SELECT LEAST();
+    ]], { 1, "Wrong number of arguments is passed to LEAST(): expected at least two, got 0" } )
+
 test:finish_test()
-- 
2.15.1

[tarantool-patches] Re: [PATCH] sql: make GREATEST/LEAST built-ins accept at least two args

[Nikita Pettik]

On Thu, Aug 29, 2019 at 01:38:47PM +0300, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/08/28 17:58]:
> > Before a46b52004 SQL implementation featured min()/max() functions
> > overloading: if one argument was passed, then aggregate version would be
> > invoked; otherwise - scalar one. We decided to get rid of it and rename
> > scalar version to LEAST()/GREATEST() correspondingly. However, assertion
> > inside their implementations has been remained: it verifies that number
> > of passed arguments is greater than 1. On the other hand, now one can
> > pass literally any number of arguments to this function, including one
> > (which results in fired mentioned assertion) and zero (which leads to
> > NULL dereference in expr.c: these functions are marked with
> > SQL_FUNC_NEEDCOLL flag, and as a consequence they are assumed to have at
> > least one argument). Firstly, let's place check that number of passed
> > arguments more than one. Secondly, let's not assume that functions with
> > SQL_FUNC_NEEDCOLL must have any arguments.
> > 
> > Closes #4453
> 
> If you introduce a new error message, why are you using it in one
> place only?

It is also used in Kirill's branch where he reworks built-ins.
I've aksed him to cherry-pick my commit. See:
https://github.com/tarantool/tarantool/commit/f8927cf3d411c88235cf532cfa3d94ee048969f2

> -- 
> Konstantin Osipov, Moscow, Russia