[tarantool-patches] Re: [PATCH 7/8] sql: clean-up affinity from SQL source code
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Jan 30 16:04:29 MSK 2019
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),
More information about the Tarantool-patches
mailing list