[tarantool-patches] Re: [PATCH v1 05/12] sql: put analyze helpers to FuncDef cache

[n.pettik]

Prior patches are OK and I guess can be pushed -
they are quite trivial (except for previous one - which
introduces persistent functions in Tarantool). Hence you
can swap order of patches (i.e. this and previous one) so
that push series of patches independently from non-trivial.
Also, please fix compilation errors which appear on Travis.

> On 8 Jul 2019, at 14:26, Kirill Shcherbatov <kshcherbatov at tarantool.org> wrote:
> 
> Previously analyze functions refer to statically defined
> service FuncDef context. We need to change this approach due we
> going to rework the builtins functions machinery in following
> patches.
> 
> Needed for #4113, #2200, #2233
> —

Unfortunately, analyze is currently disabled, so there’s
no opportunity to check functionality of this patch :)

> @@ -986,8 +956,11 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
> 		sqlVdbeAddOp3(v, OP_MakeRecord, stat_key_reg,
> 				  pk_part_count, key_reg);
> 		assert(chng_reg == (stat4_reg + 1));
> +		struct FuncDef *push_func =
> +			sqlFindFunction(sql_get(), "_sql_stat_push", 3, 0);
> +		assert(push_func != NULL);
> 		sqlVdbeAddOp4(v, OP_Function0, 1, stat4_reg, tmp_reg,
> -				  (char *)&statPushFuncdef, P4_FUNCDEF);
> +			      (char *)push_func, P4_FUNCDEF);
> 		sqlVdbeChangeP5(v, 3);
> 		sqlVdbeAddOp2(v, OP_Next, idx_cursor, next_row_addr);
> 		/* Add the entry to the stat1 table. */
> @@ -1774,3 +1747,58 @@ fail:
> 	box_txn_rollback();
> 	return -1;
> }
> +
> +/**
> + * The implementation of the sql_record() function. This function
> + * accepts a single argument of any type. The return value is a
> + * formatted database record (a blob) containing the argument
> + * value.
> + *
> + * This is used to convert the value stored in the 'sample'
> + * column of the sql_stat4 table to the record format sql uses
> + * internally.
> + */
> +static void
> +sql_record_func(sql_context * context, int argc, sql_value ** argv)

I guess there’s no need in this function at all.
Could you simply remove it?

> +{
> +	const int file_format = 1;
> +	u32 iSerial;		/* Serial type */
> +	int nSerial;		/* Bytes of space for iSerial as varint */
> +	u32 nVal;		/* Bytes of space required for argv[0] */
> +	int nRet;
> +	sql *db;
> +	u8 *aRet;
> +
> +	UNUSED_PARAMETER(argc);
> +	iSerial = sqlVdbeSerialType(argv[0], file_format, &nVal);
> +	nSerial = sqlVarintLen(iSerial);
> +	db = sql_context_db_handle(context);
> +
> +	nRet = 1 + nSerial + nVal;
> +	aRet = sqlDbMallocRawNN(db, nRet);
> +	if (aRet == 0) {
> +		context->is_aborted = true;
> +	} else {
> +		aRet[0] = nSerial + 1;
> +		putVarint32(&aRet[1], iSerial);
> +		sqlVdbeSerialPut(&aRet[1 + nSerial], argv[0], iSerial);
> +		sql_result_blob(context, aRet, nRet, SQL_TRANSIENT);
> +		sqlDbFree(db, aRet);
> +	}
> +}
> +
> +#define FUNCTION_ANALYZE(zName, nArg, xFunc) \
> +  {nArg, SQL_FUNC_CONSTANT, NULL, 0, xFunc, 0, #zName, {0},\
> +   FIELD_TYPE_ANY, false}

Could you move this macro to other FUNCTION_… ones (sqlInt.h)?

> +
> +void
> +sql_register_analyze_builtins(void)
> +{
> +	static FuncDef aAnalyzeTableFuncs[] = {

Please, use names according to our code style.