Re: [Tarantool-patches] [PATCH v1 03/13] sql: move collation to struct sql_context

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for working on this!

On 10.09.2021 18:01, imeevma@tarantool.org wrote:
> This patch makes it easier to get a collation by a function.

You could also store the opcode pointer instead of iOp. But I
suspect your main reason is to get rid of the vdbe pointer? If
yes, then why?

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 2ff7ce8f4..12dc9126b 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1159,23 +1159,13 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
>  	break;
>  }
>  
> -/* Opcode: CollSeq P1 * * P4
> - *
> - * P4 is a pointer to a CollSeq struct. If the next call to a user function
> - * or aggregate calls sqlGetFuncCollSeq(), this collation sequence will
> - * be returned. This is used by the built-in min(), max() and nullif()
> - * functions.
> +/* Opcode: SkipLoad P1 * * * *
>   *
>   * If P1 is not zero, then it is a register that a subsequent min() or
>   * max() aggregate will set to true if the current row is not the minimum or
>   * maximum.  The P1 register is initialized to false by this instruction.
> - *
> - * The interface used by the implementation of the aforementioned functions
> - * to retrieve the collation sequence set by this opcode is not available
> - * publicly.  Only built-in functions have access to this feature.
>   */
> -case OP_CollSeq: {
> -	assert(pOp->p4type==P4_COLLSEQ || pOp->p4.pColl == NULL);
> +case OP_SkipLoad: {

That is a very strange name. Couldn't OP_Bool somehow be reused?
Why is this R[p1] = false even needed?

>  	if (pOp->p1) {
>  		mem_set_bool(&aMem[pOp->p1], false);
>  	}