[tarantool-patches] Re: [PATCH v4 3/4] sql: get rid of FuncDef function hash

[Kirill Shcherbatov]

>> +#define SQL_FUNC_MAX   0x2000
> 
> Nit: extra diff.
Fixed.
>> +	/**
>> +	 * A VDBE-memory-compatible call method.
>> +	 * SQL built-ins don't use func base class "call"
>> +	 * method to provide a best performance for SQL requests.
>> +	 * Access checks are redundant, because all SQL built-ins
>> +	 * are predefined and are executed on SQL privilege level.
> 
> Which doesn’t exist yet… I asked you to document or fix it.
> Comment in source code is OK, but it should be present in
> documentation as well.
A new DocBot request in the commit message:

    @TarantoolBot document
    Title: SQL builtins priveleges
    
    All SQL built-ins are executed on SQL privilege level that is
    undefined yet.

>> +						 ">= 2", nFarg);
> 
> -> “more than one”/“at least two”
> 
> What is more, you can move introduction of ER_FUNC_WRONG_…
> to a separate auxiliary patch.
> Nit: “at least one”
Done. As an auxiliary patch.


> Nit: personally I’d not skip members and fill in them allActually I don't like it, but I don't mind so I've implemented it.

>> +	/* Some builtins are not implemented yet. */
> 
> Please, left comment describing why we really do need this check.
> I mean the fact that it disallows user to create random built-in functions.
	/*
	 * All SQL built-in(s) (stubs) are defined in a snapshot.
	 * Implementation-specific metadata is defined in
	 * sql_builtins list. When a definition were not found
	 * above, the function name is invalid, i.e. it is
	 * not built-in function.
	 */
		    "in SQL", nId, zId));
>> 				} else {
> 
> Let’s avoid call of sql_func_by_signature(). Consider refactoring:
> etc
Okey, implemented.

> Didn’t forget check that user can’t create manually built-ins?
No, I didn't. There is corresponding test in the following commit.