Re: [Tarantool-patches] [PATCH v4 2/5] sql: move diag setting to sql_func_by_signature()

[Nikita Pettik <korablev@tarantool.org>]

On 13 Jul 08:33, imeevma@tarantool.org wrote:
> After this patch, the sql_func_by_signature() function will check the
> found function and set diag if something is wrong.

Why we are doing this refactoring? I've bee begging you to provide
more descriptive commit messages for past two years.
 
> Needed for #4159
> ---
>  src/box/sql/expr.c    |  2 --
>  src/box/sql/func.c    | 18 +++++++++++++++---
>  src/box/sql/resolve.c | 23 ++---------------------
>  src/box/sql/sqlInt.h  |  7 +++++--
>  4 files changed, 22 insertions(+), 28 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index bc2182446..d0620b98c 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -3978,8 +3978,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>  			zId = pExpr->u.zToken;
>  			struct func *func = sql_func_by_signature(zId, nFarg);
>  			if (func == NULL) {
> -				diag_set(ClientError, ER_NO_SUCH_FUNCTION,
> -					 zId);
>  				pParse->is_aborted = true;
>  				break;
>  			}
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 487cdafe1..4bbe4d4b7 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -2185,11 +2185,23 @@ struct func *
>  sql_func_by_signature(const char *name, int argc)
>  {
>  	struct func *base = func_by_name(name, strlen(name));
> -	if (base == NULL || !base->def->exports.sql)
> +	if (base == NULL) {
> +		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
>  		return NULL;
> -
> -	if (base->def->param_count != -1 && base->def->param_count != argc)
> +	}
> +	if (!base->def->exports.sql) {
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +			 tt_sprintf("function %s() is not available in SQL",
> +				     name));
> +		return NULL;
> +	}
> +	int param_count = base->def->param_count;
> +	if (param_count != -1 && param_count != argc) {
> +		const char *err = tt_sprintf("%d", param_count);
> +		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT,
> +			 base->def->name, err, argc);
>  		return NULL;
> +	}
>  	return base;
>  }
>  
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 6f625dc18..10c77c491 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -598,32 +598,13 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
>  			assert(!ExprHasProperty(pExpr, EP_xIsSelect));
>  			zId = pExpr->u.zToken;
>  			nId = sqlStrlen30(zId);
> -			struct func *func = func_by_name(zId, nId);
> +			struct func *func = sql_func_by_signature(zId, n);
>  			if (func == NULL) {
> -				diag_set(ClientError, ER_NO_SUCH_FUNCTION, zId);
> -				pParse->is_aborted = true;
> -				pNC->nErr++;
> -				return WRC_Abort;
> -			}
> -			if (!func->def->exports.sql) {
> -				diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> -					 tt_sprintf("function %.*s() is not "
> -						    "available in SQL",
> -						     nId, zId));
> -				pParse->is_aborted = true;
> -				pNC->nErr++;
> -				return WRC_Abort;
> -			}
> -			if (func->def->param_count != -1 &&
> -			    func->def->param_count != n) {
> -				uint32_t argc = func->def->param_count;
> -				const char *err = tt_sprintf("%d", argc);
> -				diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT,
> -					 func->def->name, err, n);
>  				pParse->is_aborted = true;
>  				pNC->nErr++;
>  				return WRC_Abort;
>  			}
> +			assert(func->def->exports.sql);
>  			bool is_agg = func->def->aggregate ==
>  				      FUNC_AGGREGATE_GROUP;
>  			assert(!is_agg || func->def->language ==
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 37283e506..58a65acc1 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4441,8 +4441,11 @@ sql_func_flag_is_set(struct func *func, uint16_t flag)
>   * export field set true and have exactly the same signature
>   * are returned.
>   *
> - * Returns not NULL function pointer when a valid and exported
> - * to SQL engine function is found and NULL otherwise.
> + * @param name Name of the function to find.
> + * @param argc Number of arguments of the function.
> + *
> + * @retval not NULL function pointer when a function is found.
> + * @retval NULL on error and sets a diag.
>   */
>  struct func *
>  sql_func_by_signature(const char *name, int argc);
> -- 
> 2.25.1
>