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