[tarantool-patches] Re: [PATCH 7/8] sql: clean-up affinity from SQL source code

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

Thanks for the fixes! See comments below.

>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>>> index 61d73b676..c3f596d4f 100644
>>> --- a/src/box/sql/vdbe.c
>>> +++ b/src/box/sql/vdbe.c
>>> @@ -2163,9 +2151,16 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
>>>                          break;
>>>                  }
>>>          } else {
>>> -               /* Neither operand is NULL.  Do a comparison. */
>>> -               affinity = pOp->p5 & AFFINITY_MASK;
>>> -               if (affinity>=AFFINITY_INTEGER) {
>>> +               /*
>>> +                * Neither operand is NULL. Do a comparison.
>>> +                * 15 is 1111 in a binary form.
>>> +                * Since all existing types can be fitted in 4 bits
>>> +                * (field_type_MAX == 10), it is enough to 'and'
>>> +                * p5 with this constant. Node that all other flags
>>> +                * that can be stored in p5 are >= 16.
>>> +                */
>>> +               affinity = pOp->p5 & 15;
>>
>> 4. Honestly, I did not expect it from you. Please, never use such
>> constants and assumptions about them in code. field_type_MAX = 10 can
>> be changed any day (for example, when we introduce decimal as a native
>> Tarantool data type, or decide to add datetime). In fact, it is already
>> not 10 - it is 9.
> 
> Oh, cmon. This place will explode only if number of types exceed 15.
> Not so soon, I guess (when was the last time we introduced new type?).
> So it won’t be my problem by the time it occurs.
> 
>> I propose you to either come up with a better solution or to use one of
>> my solutions:
>>
>> * return AFFINITY_MASK as P5_FIELD_TYPE_MASK, and add a static assertion
>>   that P5_FIELD_TYPE_MASK < (1 << bit_count(field_type)).
>>
>>   Or write in field_def.h something like this:
>>
>>       static_assert(bit_count(field_type) <= 4,
>>                     "size of enum field_type is used in "\
>>                     "VdbeOp.p5 to fit it into 4 bits”);
> 
> To be honest I don’t understand these bit assertions.
> Why can’t we simply check that field_type_MAX <= 15?
> 
> diff --git a/src/box/field_def.c b/src/box/field_def.c
> index b34d2ccd9..1a8074650 100644
> --- a/src/box/field_def.c
> +++ b/src/box/field_def.c
> @@ -33,6 +33,13 @@
>   #include "trivia/util.h"
>   #include "key_def.h"
>   
> +/**
> + * For detailed explanation see context of OP_Eq, OP_Lt etc
> + * opcodes in vdbe.c.
> + */
> +static_assert(field_type_MAX <= 15, "values of enum field_type "\
> +             "should fit into 4 bits of VdbeOp.p5");
> +
> 
> Also, as you may notice, I placed it to field_def.c
> Seems that field_def.h is used to generate module.h,
> which in turn doesn’t tolerate this kind of assert.

field_def.h is not copied to module.h entirely. Only
the code between /** \cond public */ and /** \endcond public */.
Please, put this assertion below /** \endcond public */.

> 
>>   To get bit_count during compilation you can specify it explicitly right
>>   after field_type_MAX in enum field_type so as to change it always
>>   together with field_type_MAX.
>>
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 53445faf5..1f28893f6 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
>>> @@ -419,16 +419,15 @@ codeApplyAffinity(Parse * pParse, int base, int n, char *zAff)
>>>   static void
>>>   updateRangeAffinityStr(Expr * pRight,  /* RHS of comparison */
>>>                         int n,           /* Number of vector elements in comparison */
>>> -                      char *zAff)      /* Affinity string to modify */
>>> +                      enum field_type *zAff)   /* Affinity string to modify */
>>
>> 6. The same. In many other places too. Where you changed type and
>> usages of a variable, but kept its name.
> 
> I did it on purpose - to reduce diff, so that separate functional
> changes and non-functional such as renaming. Now fixed.

Small diff is good, but lets do not overemphasize. This patch states
that affinity is removed, and not only as a functionality, but from
the source code, but before your fixes it actually was not. It
complicated reading and understanding of the code.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 80d2fd0aa..358b69754 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2164,17 +2151,24 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
>  			break;
>  		}
>  	} else {
> -		/* Neither operand is NULL.  Do a comparison. */
> -		affinity = pOp->p5 & AFFINITY_MASK;
> -		if (affinity>=AFFINITY_INTEGER) {
> +		/*
> +		 * Neither operand is NULL. Do a comparison.
> +		 * 15 is 1111 in a binary form.
> +		 * Since all existing types can be fitted in 4 bits
> +		 * (field_type_MAX == 10), it is enough to 'and'
> +		 * p5 with this constant. Node that all other flags
> +		 * that can be stored in p5 are >= 16.

Firstly, as I said in the previous review, field_type_MAX == 9.
Secondly, please, do not use a constant. All of them (15, 111, 10, 16).
Instead of this huge comment full of constants just create a enum
value with an appropriate name, like FIELD_TYPE_MASK = 15, it is
even ok to put this name into field_def.h. Thirdly, not all p5 flags
are >= 16. But for these concrete opcodes it is so, luckily. At
least we have OPFLAG_NCHANGE, OPFLAG_EPHEM, OPFLAG_SEEKEQ etc -
they all are <= 15.

> +		 */
> +		enum field_type type = pOp->p5 & 15;
> +		if (sql_type_is_numeric(type)) {
>  			if ((flags1 | flags3)&MEM_Str) {
>  				if ((flags1 & (MEM_Int|MEM_Real|MEM_Str))==MEM_Str) {
> -					applyNumericAffinity(pIn1,0);
> +					mem_apply_numeric_type(pIn1, 0);
>  					testcase( flags3!=pIn3->flags); /* Possible if pIn1==pIn3 */
>  					flags3 = pIn3->flags;
>  				}
>  				if ((flags3 & (MEM_Int|MEM_Real|MEM_Str))==MEM_Str) {
> -					if (applyNumericAffinity(pIn3,0) != 0) {
> +					if (mem_apply_numeric_type(pIn3, 0) != 0) {
>  						diag_set(ClientError,
>  							 ER_SQL_TYPE_MISMATCH,
>  							 sqlite3_value_text(pIn3),