On 19.08.2021 11:35, Vladimir Davydov via Tarantool-patches wrote: > On Wed, Aug 18, 2021 at 05:34:59PM +0300, imeevma@tarantool.org wrote: >> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c >> index 0d998506c..369d9e1dd 100644 >> --- a/extra/mkkeywordhash.c >> +++ b/extra/mkkeywordhash.c >> @@ -184,7 +184,6 @@ static Keyword aKeywordTable[] = { >> { "BLOB", "TK_STANDARD", true }, >> { "BINARY", "TK_ID", true }, >> { "CALL", "TK_STANDARD", true }, >> - { "CHAR", "TK_CHAR", true }, >> { "CHARACTER", "TK_ID", true }, >> { "CONDITION", "TK_STANDARD", true }, >> { "CONNECT", "TK_STANDARD", true }, >> @@ -251,6 +250,42 @@ static Keyword aKeywordTable[] = { >> { "LEADING", "TK_LEADING", true }, >> { "TRAILING", "TK_TRAILING", true }, >> { "BOTH", "TK_BOTH", true }, >> + { "ABS", "TK_ABS", true }, >> + { "AVG", "TK_AVG", true }, >> + { "CHAR", "TK_CHAR", true }, >> + { "CHAR_LENGTH", "TK_CHAR_LEN", true }, >> + { "CHARACTER_LENGTH", "TK_CHAR_LEN", true }, >> + { "COALESCE", "TK_COALESCE", true }, >> + { "COUNT", "TK_COUNT", true }, >> + { "GREATEST", "TK_GREATEST", true }, >> + { "GROUP_CONCAT", "TK_GROUP_CONCAT",true }, >> + { "HEX", "TK_HEX", true }, >> + { "IFNULL", "TK_IFNULL", true }, >> + { "LEAST", "TK_LEAST", true }, >> + { "LENGTH", "TK_LENGTH", true }, >> + { "LIKELIHOOD", "TK_LIKELIHOOD", true }, >> + { "LIKELY", "TK_LIKELY", true }, >> + { "LOWER", "TK_LOWER", true }, >> + { "MAX", "TK_MAX", true }, >> + { "MIN", "TK_MIN", true }, >> + { "NULLIF", "TK_NULLIF", true }, >> + { "POSITION", "TK_POSITION", true }, >> + { "PRINTF", "TK_PRINTF", true }, >> + { "QUOTE", "TK_QUOTE", true }, >> + { "RANDOM", "TK_RANDOM", true }, >> + { "RANDOMBLOB", "TK_RANDOMBLOB", true }, >> + { "ROUND", "TK_ROUND", true }, >> + { "ROW_COUNT", "TK_ROW_COUNT", true }, >> + { "SOUNDEX", "TK_SOUNDEX", true }, >> + { "SUBSTR", "TK_SUBSTR", true }, >> + { "SUM", "TK_SUM", true }, >> + { "TOTAL", "TK_TOTAL", true }, >> + { "TYPEOF", "TK_TYPEOF", true }, >> + { "UNICODE", "TK_UNICODE", true }, >> + { "UNLIKELY", "TK_UNLIKELY", true }, >> + { "UPPER", "TK_UPPER", true }, >> + { "VERSION", "TK_VERSION", true }, >> + { "ZEROBLOB", "TK_ZEROBLOB", true }, > > Should be sorted? They all made reserved (last true) - that's one of a problems in a patch. > >> }; >> >> /* Number of keywords */ >> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y >> index bd041e862..cb2e627db 100644 >> --- a/src/box/sql/parse.y >> +++ b/src/box/sql/parse.y >> @@ -1172,27 +1172,506 @@ 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 = >> - tt_sprintf("Number of arguments to function %.*s", X.n, X.z); >> - diag_set(ClientError, ER_SQL_PARSER_LIMIT, err, Y->nExpr, >> - pParse->db->aLimit[SQL_LIMIT_FUNCTION_ARG]); >> +expr(A) ::= ABS(X) LP distinct(D) exprlist(Y) RP(E). { >> + if (Y == NULL || Y->nExpr != 1) { >> + int n = Y == NULL ? 0 : Y->nExpr; >> + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "ABS", "1", n); >> pParse->is_aborted = true; >> + return; >> } >> A.pExpr = sqlExprFunction(pParse, Y, &X); >> - spanSet(&A,&X,&E); >> - if( D==SF_Distinct && A.pExpr ){ >> + spanSet(&A, &X, &E); >> + if(D == SF_Distinct && A.pExpr) >> A.pExpr->flags |= EP_Distinct; >> +} >> + >> +expr(A) ::= AVG(X) LP distinct(D) exprlist(Y) RP(E). { >> + if (Y == NULL || Y->nExpr != 1) { >> + int n = Y == NULL ? 0 : Y->nExpr; >> + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "AVG", "1", n); >> + pParse->is_aborted = true; >> + return; >> } >> + A.pExpr = sqlExprFunction(pParse, Y, &X); >> + spanSet(&A, &X, &E); >> + if(D == SF_Distinct && A.pExpr) >> + A.pExpr->flags |= EP_Distinct; >> } >> >> -/* >> - * type_func(A) ::= DATE(A) . >> - * type_func(A) ::= DATETIME(A) . >> - */ >> -type_func(A) ::= CHAR(A) . >> -expr(A) ::= type_func(X) LP distinct(D) exprlist(Y) RP(E). { >> +expr(A) ::= CHAR(X) LP distinct(D) exprlist(Y) RP(E). { >> + if (Y != NULL && Y->nExpr > > pParse->db->aLimit[SQL_LIMIT_FUNCTION_ARG]) { >> + const char *str = tt_sprintf("from %d to %d", 0, >> + > pParse->db->aLimit[SQL_LIMIT_FUNCTION_ARG]); >> + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "CHAR", str, > Y->nExpr); >> + pParse->is_aborted = true; >> + return; >> + } >> + A.pExpr = sqlExprFunction(pParse, Y, &X); >> + spanSet(&A, &X, &E); >> + if(D == SF_Distinct && A.pExpr) >> + A.pExpr->flags |= EP_Distinct; >> +} >> + >> +expr(A) ::= CHAR_LEN(X) LP distinct(D) exprlist(Y) RP(E). { >> + if (Y == NULL || Y->nExpr != 1) { >> + int n = Y == NULL ? 0 : Y->nExpr; >> + const char *name = X.n == strlen("CHAR_LENGTH") ? "CHAR_LENGTH" : >> + "CHARACTER_LENGTH"; >> + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name, "1", n); >> + pParse->is_aborted = true; >> + return; >> + } >> + A.pExpr = sqlExprFunction(pParse, Y, &X); >> + spanSet(&A, &X, &E); >> + if(D == SF_Distinct && A.pExpr) >> + A.pExpr->flags |= EP_Distinct; >> +} >> + >> +expr(A) ::= COALESCE(X) LP distinct(D) exprlist(Y) RP(E). { >> + if (Y == NULL || Y->nExpr < 2 || >> + Y->nExpr > pParse->db->aLimit[SQL_LIMIT_FUNCTION_ARG]) { >> + int n = Y == NULL ? 0 : Y->nExpr; >> + const char *str = tt_sprintf("from %d to %d", 2, >> + > pParse->db->aLimit[SQL_LIMIT_FUNCTION_ARG]); >> + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "COALESCE", str, n); >> + pParse->is_aborted = true; >> + return; >> + } >> + A.pExpr = sqlExprFunction(pParse, Y, &X); >> + spanSet(&A, &X, &E); >> + if(D == SF_Distinct && A.pExpr) >> + A.pExpr->flags |= EP_Distinct; >> +} > > Can you the code to a helper function to avoid copy-paste? Yup, a lot of copy-paste. Hard to distibgush the core of each. This all could make significantly shorter: ------------------------------------------------------------------- expr(A) ::= builtin_fn(X) LP distinct(D) exprlist(Y) RP(E). { /* variable number of arguments */ int args = X.args, maxargs = X.maxargs; int n = Y == NULL ? 0 : Y->nExpr; if (X.vararg) { int limit_args = pParse->db->aLimit[SQL_LIMIT_FUNCTION_ARG]; /* less than minimal # of expected or more than maximal */ if (n < args || n > limit_args) { const char *str = tt_sprintf("from %d to %d", 0, limit_args); diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, TOKEN_STR(X.token), str, Y->nExpr); pParse->is_aborted = true; return; } } else { /* not expected number - args */ if (maxargs == 0 && n != args) { diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, TOKEN_STR(X.token), tt_sprintf("%d", args), n); pParse->is_aborted = true; return; } /* from args till maxargs */ if (n < args || (maxargs != 0 && n > maxargs)) {/* or more than expected */ const char *str = tt_sprintf("from %d to %d", args, maxargs); diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, TOKEN_STR(X.token), str, n); pParse->is_aborted = true; return; } } A.pExpr = sql_expr_new_built_in(pParse, Y, &X.token, X.id); spanSet(&A, &X.token, &E); if (D == SF_Distinct && A.pExpr) A.pExpr->flags |= EP_Distinct; } %type builtin_fn {struct builtin_fn_def} builtin_fn(A) ::= ABS(T). { FUNC_ARG_N(A, T, TK_ABS, 1); } builtin_fn(A) ::= AVG(T). { FUNC_ARG_N(A, T, TK_AVG, 1); } builtin_fn(A) ::= CHAR(T). { FUNC_ARG_VAR(A, T, TK_CHAR); } builtin_fn(A) ::= CHAR_LEN(T). { FUNC_ARG_N(A, T, TK_CHAR_LENGTH, 1); } builtin_fn(A) ::= COALESCE(T). { FUNC_ARG_VAR_N(A, T, TK_COALESCE, 2); } /*builtin_fn(A) ::=(T) COUNT. { FUNC_ARG_N(A, T, TK_COUNT, 1); }*/ builtin_fn(A) ::= GREATEST(T). { FUNC_ARG_VAR_N(A, T, TK_GREATEST, 2); } builtin_fn(A) ::= GROUP_CONCAT(T). { FUNC_ARG_N_M(A, T, TK_GROUP_CONCAT, 1, 2); } .... ------------------------------------------------------------------- More details you could see in apatch attached. > >> diff --git a/test/box/tx_man.result b/test/box/tx_man.result >> index 786d7fc30..b99fbc2ca 100644 >> --- a/test/box/tx_man.result >> +++ b/test/box/tx_man.result >> @@ -2129,11 +2129,11 @@ tx1:rollback() >> >> -- gh-6095: SQL query may crash in MVCC mode if it involves ephemeral > spaces. >> -- >> -box.execute([[ CREATE TABLE test (id INT NOT NULL PRIMARY KEY, count > INT NOT NULL)]]) >> +box.execute([[ CREATE TABLE test (id INT NOT NULL PRIMARY KEY, "COUNT" > INT NOT NULL)]]) >> | --- >> | - row_count: 1 >> | ... >> -box.execute([[ UPDATE test SET count = count + 1 WHERE id = 0 ]]) >> +box.execute([[ UPDATE test SET "COUNT" = "COUNT" + 1 WHERE id = 0 ]]) > > This looks bad. MySQL and PostgreSQL allow that. I've fixed it in a patch suggested... > >> | --- >> | - row_count: 0 >> | ... >> diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua >> index e7b35c9d9..7dd85025a 100755 >> --- a/test/sql-tap/func.test.lua >> +++ b/test/sql-tap/func.test.lua >> @@ -68,7 +68,7 @@ test:do_catchsql_test( >> SELECT length(*) FROM tbl1 ORDER BY t1 >> ]], { >> -- >> - 1, "Wrong number of arguments is passed to LENGTH(): expected > 1, got 0" >> + 1, "Syntax error at line 1 near '*'" > > This is probably okay. > >> -- >> }) >> >> @@ -2483,7 +2483,7 @@ test:do_catchsql_test( >> SELECT coalesce() >> ]], { >> -- >> - 1, "Wrong number of arguments is passed to COALESCE(): expected > at least two, got 0" >> + 1, "Wrong number of arguments is passed to COALESCE(): expected > from 2 to 127, got 0" > > And this too. > [Patch attached is for branch imeevma/gh-6105-check-types-no-test] Best Regards, Timur