[tarantool-patches] Re: [PATCH v2 7/8] sql: get rid of FuncDef function hash

[Kirill Shcherbatov]

>> It has a sql-specific method :call and :finalize typica while
> 
> Nit: typic? Please, consider using spell-checker.
>> Following tests using sql_create_function are broken now.
>> They would be fixed in the following commit:
> 
> Nit: are going to be fixed in the next commit.
Fixed.

>> +	/** A call method for a function. */
> 
> Add explanation why we don’t use basic (i.e. call
> method of base class) “call” method. Furthermore,
> built-ins are called bypassing func_access_check()
> function. This should be either documented or fixed.
	/**
	 * A VDBE-memory-compatible call method for a function.
	 * SQL Builting functions doesn't use base class call
	 * method to gain the best possible performance for SQL
	 * requests. As builtin functions are predefined and
	 * non of them modifie schema, access checks are
	 * redundant, functions have the same execution
	 * privileges as SQL.
	 */
	void (*call)(sql_context *ctx, int argc, sql_value **argv);

>> +	/** Finalize method (only for aggregate function). */
>> +	void (*finalize)(sql_context *ctx);
> 
> We hold aggregate attribute in func_def; I guess
> lua/C aggregate are not going to be implemented
> soon (or someday at all); so, considering these
> points, why not to move this method to the base vtab?
Verbally discussed. It is vdbe-memory-oriented API, so
it shouldn't be in abstract function's vtab.

>> +/**
>> + * A SQL method to find a function in a hash by it's name and
> 
> Nit: it’s (it is) -> its.
>> + * Returns not NULL function pointer when a valid and exported
>> + * to SQL engine function was found and NULL otherwise.
> 
> Nit: was found -> is found.
Fixed.

>> +static inline bool
>> +sql_func_flag_test(struct func *func, uint16_t flag)
> 
> At least I would rename it to sql_func_test_flag() or
> sql_builtin_flag_is_set() or smth like that.
Verbally discussed, I believe that it is the most representative name for
this helper.

>> +/**
>> + * Memory cell pMem contains the context of an aggregate function.
> 
> Nit: pMem -> mem.
Fixed.

>> -static void
>> +MAYBE_UNUSED static void
> 
> Is this change really related to the patch? Same for
> all other added MAYBE_UNUSED modifiers.
I don't initialze analyze functions because the are currently
unused, but I don't like to remove their implementation.

>> 	assert(func != NULL);
> 
> Instead of asserting func existence let’s raise an error: now
> user can remove built-in function from hash - there’s no
> protection to avoid such cases. Note that such opportunity
> (removing built-ins from cache) has been introduced by moving
> built-ins to the global func cache. In this case, assertion will fail
> in debug mode and lead to unpredictable consequences in
> release mode. Another option is to introduce mentioned protection,
> i.e. disallow user to delete functions which are declared with
> FUNC_LANGUAGE_SQL_BUILTIN flag.
I've introduce remove protection in alter.cc

>> +			    func->def->aggregate == FUNC_AGGREGATE_GROUP) {
> 
> Why do we need check on aggregation type?
Works also without it, thanks.

>> +		diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
>> +			 "body and is_sandboxed options are not compatible "
>> +			 "with SQL language”);
> 
> Why not move this check to func_def_new_from_tuple() ?
Already there.

>> +	func->flags = sql_builtins[idx].flags;
>> +	func->user_data = sql_builtins[idx].user_data;
>> +	func->call = sql_builtins[idx].call;
>> +	func->finalize = sql_builtins[idx].finalize;
>> +	func->signature_mask = sql_builtins[idx].signature_mask;
>> +	func->base.vtab = &func_sql_builtin_vtab;
>> +	func->base.def = def;
>> +
> Why not memcpy?
Because it is partial func def initialization. Can't just mecpy it.

> Nit: double semicolon at the end of string.
Fixed.
> 
> Can aggregate function be non-builtin?
Not for now, but I don't like to rely on it so I make all
corresponding checks.

> SQL_FUNC_SLOCHNG seems to be unused.
> So, you can simply remove it.
Removed in separate commit.

>> +	(*((struct func_sql_builtin *)pCtx->func)->call)(pCtx, pCtx->argc,
>> +							 pCtx->argv);
> 
> Nit: please, split it into two steps: type cast and function call.
Done.

> user_data is used only for min/max functions. Let’s consider removing it.
Removed user_data at all.

>> +	assert(p != NULL && p->func != NULL &&
>> +	       p->func->def->language == FUNC_LANGUAGE_SQL_BUILTIN &&
>> +	       p->func->def->aggregate == FUNC_AGGREGATE_GROUP);
> 
> Please, split this assert into three ones. IMHO it would
> improve code readability.
Done.

>> -	if (ALWAYS(pFunc && pFunc->xFinalize)) {
>> +	if (ALWAYS(func != NULL &&
> 
> Please, move assert inside if condition to a separate stmt(s):
Done.