[tarantool-patches] Re: [PATCH v5 3/6] sql: introduce tuple_fetcher class

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes! See 10 comments below.

> diff --git a/src/box/sql.h b/src/box/sql.h
> index 15ef74b19..3aaeb2274 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -386,6 +386,52 @@ sql_src_list_entry_name(const struct SrcList *list, int i);
>  void
>  sqlSrcListDelete(struct sql *db, struct SrcList *list);
>  
> +/**
> + * Auxilary VDBE structure to speed-up tuple data field access.

1. 'Auxilary' -> 'Auxiliary'. Looks like you still did not install
spell checker, as I asked in some previous patchsets. Please, do it.

> + * A memory allocation that manage this structure must have
> + * trailing unused bytes that extends the last 'slots' array.
> + * The amount of reserved memory should correspond to the problem
> + * to be solved and is usually equal to the greatest number of
> + * fields in the tuple.
> + *
> + * +------------------------+
> + * |  struct tuple_fetcher  |
> + * +------------------------+
> + * |     RESERVED MEMORY    |
> + * +------------------------+
> + */
> +struct tuple_fetcher {
> +	/** Tuple pointer or NULL when undefined. */
> +	struct tuple *tuple;
> +	/** Tuple data pointer. */
> +	const char *data;
> +	/** Tuple data size. */
> +	uint32_t data_sz;
> +	/** Count of fields in tuple. */
> +	uint32_t field_count;
> +	/**
> +	 * Index of the rightmost initialized slot in slots
> +	 * array.
> +	 */
> +	uint32_t rightmost_slot;
> +	/**
> +	 * Array of offsets of tuple fields.
> +	 * Only values <= rightmost_slot are valid.
> +	 */
> +	uint32_t slots[1];
> +};
> +
> +/**
> + * Initialize a new tuple_fetcher instance with given tuple
> + * data.
> + * @param fetcher The tuple_fetcher instance to initialize.
> + * @param tuple The tuple object pointer or NULL when undefined.
> + * @param data The tuple data (is always defined).
> + * @param data_sz The size of tuple data (is always defined).
> + */
> +void
> +tuple_fetcher_create(struct tuple_fetcher *fetcher, struct tuple *tuple,
> +		     const char *data, uint32_t data_sz);

2. Great, I like this function, especially that it does not have
any single 'if' - perfect case for processor's conveyor parallelizing.

But the API is ambiguous - what should a user pass here? Always both
tuple and data, or tuple and different data, or tuple and its data,
or tuple + NULL vs NULL + data?

Please, expose two public functions: tuple_fetcher_prepare_data and
tuple_fetcher_prepare_tuple. Each takes either const char *data or
struct tuple *tuple. Internally they call tuple_fetcher_create with
both tuple and data.

Once you did it, you can stop calling tarantoolsqlPayloadFetch in
OP_Column and do either tuple_fetcher_prepare_tuple(pCrsr->last_tuple)
in case it is a normal cursor, or do
tuple_fetcher_prepare_data(pReg->z, pReg->n) in case of a pseudo cursor.

>  
>  #if defined(__cplusplus)
>  } /* extern "C" { */
> diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
> index 2b04d961e..bb0f8f1a4 100644
> --- a/src/box/sql/tarantoolInt.h
> +++ b/src/box/sql/tarantoolInt.h
> @@ -28,7 +28,8 @@ const void *tarantoolsqlPayloadFetch(BtCursor * pCur, u32 * pAmt);
>   *         offset to @a fieldno.
>   */
>  const void *
> -tarantoolsqlTupleColumnFast(BtCursor *pCur, u32 fieldno, u32 *field_size);
> +tarantool_tuple_field_fast(struct tuple *tuple, uint32_t fieldno,
> +			   uint32_t *field_size);

3. I suggest you to make this function method of the fetcher and static inline
right above tuple_fetcher_fetch(). It is not needed in any other place.

>  
>  int tarantoolsqlFirst(BtCursor * pCur, int *pRes);
>  int tarantoolsqlLast(BtCursor * pCur, int *pRes);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 5d37f63fb..33c5458cd 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -612,6 +612,119 @@ mem_type_to_str(const struct Mem *p)
>  	}
>  }
>  
> +/**
> + * Fetch field by field_idx using tuple_fetcher and store result
> + * in dest_mem.
> + * @param fetcher The initialized tuple_fetcher instance to use.
> + * @param field_idx The id of the field to fetch.
> + * @param field_type The destination memory field type is used
> + *                   when fetch result type is undefined.

4. The only reason to make tuple_fetcher_fetch() a separate function
was to get rid of this parameter and related code, keeping it in
OP_Column. Please, do it. Remove the parameter and keep it in
OP_Column.

> + * @param default_val_mem The value to return when fetcher's data
> + *                        lacks field by given @a field_idx.

5. Why would ever need that parameter for OP_Fetch? It is not a
task of a fetcher to return default fields IMO. It should only fetch
a field, not replace it with a default value.

> + * @param[out] dest_mem The memory variable to store result.
> + * @retval SQL_OK Status code in case of success.
> + * @retval sql_ret_code Error code otherwise.
> + */
> +static int
> +tuple_fetcher_fetch(struct tuple_fetcher *fetcher, uint32_t field_idx,
> +		    enum field_type field_type, struct Mem *default_val_mem,
> +		    struct Mem *dest_mem)
> @@ -2614,19 +2722,21 @@ case OP_Column: {>  
> +/* Opcode: Fetch P1 P2 P3 P4 P5
> + * Synopsis: r[P3]=PX
> + *
> + * Interpret the data that P1 points as an initialized

6. "Interpret data P1 points at as an initialized ...".

> + * tuple_fetcher object.
> + *
> + * Fetch the P2-th column from their tuple. The value extracted

7. 'Their' is for plural. Use 'its'.

> + * is stored in register P3.
> + *
> + * If the column contains fewer than P2 fields, then extract
> + * a NULL. Or, if the P4 argument is a P4_MEM use the value of
> + * the P4 argument as the result.

8. You never generate OP_Fetch with a default value. Please,
drop P4 argument. As well as P5. They are never used, even in the
last patch.

> + *
> + * Value of P5 register is an 'expected' destination value type.

9. It is not 'expected'. I can't pass here FIELD_TYPE_STRING and get
a string value despite an actually stored type. It is rather a flag,
that you need to transform 'int' into 'number'. Anyway, it should be
dropped.

> + */
> +case OP_Fetch: {
> +	struct tuple_fetcher *fetcher =
> +		(struct tuple_fetcher *) p->aMem[pOp->p1].u.p;
> +	uint32_t field_idx = pOp->p2;
> +	struct Mem *dest_mem = &aMem[pOp->p3];
> +	struct Mem *default_val_mem =
> +		pOp->p4type == P4_MEM ? pOp->p4.pMem : NULL;
> +	enum field_type field_type = pOp->p5;
> +	memAboutToChange(p, dest_mem);
> +	rc = tuple_fetcher_fetch(fetcher, field_idx, field_type,
> +			         default_val_mem, dest_mem);
> +	if (rc != SQL_OK)
> +		goto abort_due_to_error;
> +	UPDATE_MAX_BLOBSIZE(dest_mem);
10. Why is not this command a part of tuple_fetcher_fetch()?

> +	REGISTER_TRACE(p, pOp->p3, dest_mem);
> +	break;
> +}