Re: [Tarantool-patches] [PATCH v5 5/8] box, datetime: datetime comparison for indices

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

18.08.2021 02:43, Safin Timur пишет:
> On 17.08.2021 15:16, Serge Petrenko wrote:
>>
>>
>> 16.08.2021 02:59, Timur Safin via Tarantool-patches пишет:
>>> * storage hints implemented for datetime_t values;
>>> * proper comparison for indices of datetime type.
>>>
>>> Part of #5941
>>> Part of #5946
>>
>>
>> Please, add a docbot request stating that it's now possible to store
>> datetime values in spaces and create indexed datetime fields.
>
> Will use something like that:
>
> @TarantoolBot document
>
> Title: Storage support for datetime values
>
> It's now possible to store datetime values in spaces and create
> indexed datetime fields.
>
> Please refer to https://github.com/tarantool/tarantool/discussions/6244
> for more detailed description of a storage schema.

Looks ok to me.

>
>>
>>
>>> ---
>>>   src/box/field_def.c           | 18 ++++++++
>>>   src/box/field_def.h           |  3 ++
>>>   src/box/memtx_space.c         |  3 +-
>>>   src/box/tuple_compare.cc      | 57 ++++++++++++++++++++++++++
>>>   src/box/vinyl.c               |  3 +-
>>>   test/engine/datetime.result   | 77 
>>> +++++++++++++++++++++++++++++++++++
>>>   test/engine/datetime.test.lua | 35 ++++++++++++++++
>>>   7 files changed, 192 insertions(+), 4 deletions(-)
>>>   create mode 100644 test/engine/datetime.result
>>>   create mode 100644 test/engine/datetime.test.lua
>>>
>>> diff --git a/src/box/field_def.c b/src/box/field_def.c
>>> index 2682a42ee..97033d0bb 100644
>>> --- a/src/box/field_def.c
>>> +++ b/src/box/field_def.c
>>> @@ -194,3 +194,21 @@ field_type_by_name(const char *name, size_t len)
>>>           return FIELD_TYPE_ANY;
>>>       return field_type_MAX;
>>>   }
>>> +
>>> +const bool field_type_index_allowed[] =
>>> +    {
>>> +    /* [FIELD_TYPE_ANY]      = */ false,
>>> +    /* [FIELD_TYPE_UNSIGNED] = */ true,
>>> +    /* [FIELD_TYPE_STRING]   = */ true,
>>> +    /* [FIELD_TYPE_NUMBER]   = */ true,
>>> +    /* [FIELD_TYPE_DOUBLE]   = */ true,
>>> +    /* [FIELD_TYPE_INTEGER]  = */ true,
>>> +    /* [FIELD_TYPE_BOOLEAN]  = */ true,
>>> +    /* [FIELD_TYPE_VARBINARY]= */ true,
>>> +    /* [FIELD_TYPE_SCALAR]   = */ true,
>>> +    /* [FIELD_TYPE_DECIMAL]  = */ true,
>>> +    /* [FIELD_TYPE_UUID]     = */ true,
>>> +    /* [FIELD_TYPE_ARRAY]    = */ false,
>>> +    /* [FIELD_TYPE_MAP]      = */ false,
>>> +    /* [FIELD_TYPE_DATETIME] = */ true,
>>> +};
>>
>>
>> You wouldn't need that array if you moved
>> FIELD_TYPE_DATETIME above FIELD_TYPE_ARRAY
>> in the previous commit.
>>
>> Please, do so.
>
> Yes, will change order and also move all field support code to this 
> patch (as Vova recommends).
>
>>
>>
>>> diff --git a/src/box/field_def.h b/src/box/field_def.h
>>> index 120b2a93d..bd02418df 100644
>>> --- a/src/box/field_def.h
>>> +++ b/src/box/field_def.h
>>> @@ -120,6 +120,9 @@ extern const uint32_t field_ext_type[];
>>>   extern const struct opt_def field_def_reg[];
>>>   extern const struct field_def field_def_default;
>>> +/** helper table for checking allowed indices for types */
>>> +extern const bool field_type_index_allowed[];
>>> +
>>>   /**
>>>    * @brief Field definition
>>>    * Contains information about of one tuple field.
>>> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
>>> index b71318d24..1ab16122e 100644
>>> --- a/src/box/memtx_space.c
>>> +++ b/src/box/memtx_space.c
>>> @@ -748,8 +748,7 @@ memtx_space_check_index_def(struct space *space, 
>>> struct index_def *index_def)
>>>       /* Check that there are no ANY, ARRAY, MAP parts */
>>>       for (uint32_t i = 0; i < key_def->part_count; i++) {
>>>           struct key_part *part = &key_def->parts[i];
>>> -        if (part->type <= FIELD_TYPE_ANY ||
>>> -            part->type >= FIELD_TYPE_ARRAY) {
>>> +        if (!field_type_index_allowed[part->type]) {
>>>               diag_set(ClientError, ER_MODIFY_INDEX,
>>>                    index_def->name, space_name(space),
>>>                    tt_sprintf("field type '%s' is not supported",
>>> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
>>> index 9a69f2a72..110017853 100644
>>> --- a/src/box/tuple_compare.cc
>>> +++ b/src/box/tuple_compare.cc
>>> @@ -538,6 +538,8 @@ tuple_compare_field_with_type(const char 
>>> *field_a, enum mp_type a_type,
>>>                              field_b, b_type);
>>>       case FIELD_TYPE_UUID:
>>>           return mp_compare_uuid(field_a, field_b);
>>> +    case FIELD_TYPE_DATETIME:
>>> +        return mp_compare_datetime(field_a, field_b);
>>>       default:
>>>           unreachable();
>>>           return 0;
>>> @@ -1538,6 +1540,21 @@ func_index_compare_with_key(struct tuple 
>>> *tuple, hint_t tuple_hint,
>>>   #define HINT_VALUE_DOUBLE_MAX    (exp2(HINT_VALUE_BITS - 1) - 1)
>>>   #define HINT_VALUE_DOUBLE_MIN    (-exp2(HINT_VALUE_BITS - 1))
>>> +/**
>>> + * We need to squeeze 64 bits of seconds and 32 bits of nanoseconds
>>> + * into 60 bits of hint value. The idea is to represent wide enough
>>> + * years range, and leave the rest of bits occupied from 
>>> nanoseconds part:
>>> + * - 36 bits is enough for time range of [208BC..4147]
>>> + * - for nanoseconds there is left 24 bits, which are MSB part of
>>> + *   32-bit value
>>> + */
>>> +#define HINT_VALUE_SECS_BITS    36
>>> +#define HINT_VALUE_NSEC_BITS    (HINT_VALUE_BITS - 
>>> HINT_VALUE_SECS_BITS)
>>> +#define HINT_VALUE_SECS_MAX    ((1LL << HINT_VALUE_SECS_BITS) - 1)
>>
>> Am I missing something?
>> n bits may store values from (-2^(n-1)) to 2^(n-1)-1
>>
>> should be (1LL << (HINT_VALUE_SECS_BITS -1))  - 1 ?
>>
>>
>>> +#define HINT_VALUE_SECS_MIN    (-(1LL << HINT_VALUE_SECS_BITS))
>>
>>
>>
>>
>> should be
>>
>> #define HINT_VALUE_SECS_MIN    (-(1LL << (HINT_VALUE_SECS_BITS - 1)))
>>
>> ?
>>
>
> Yes, my definition made sense only when used as a mask (in prior 
> version of a code). Thus did not take into consideration a sign bit. 
> You absolutely correct that if seconds are signed then we have lesser 
> number of bits, and your definitions of 
> HINT_VALUE_SECS_MAX/HINT_VALUE_SECS_MIN should be used.
>
>>
>>> +#define HINT_VALUE_NSEC_SHIFT (sizeof(int) * CHAR_BIT - 
>>> HINT_VALUE_NSEC_BITS)
>>> +#define HINT_VALUE_NSEC_MAX    ((1ULL << HINT_VALUE_NSEC_BITS) - 1)
>>> +
>>>   /*
>>>    * HINT_CLASS_BITS should be big enough to store any mp_class value.
>>>    * Note, ((1 << HINT_CLASS_BITS) - 1) is reserved for HINT_NONE.
>>> @@ -1630,6 +1647,25 @@ hint_uuid_raw(const char *data)
>>>       return hint_create(MP_CLASS_UUID, val);
>>>   }
>>> +static inline hint_t
>>> +hint_datetime(struct datetime *date)
>>> +{
>>> +    /*
>>> +     * Use at most HINT_VALUE_SECS_BITS from datetime
>>> +     * seconds field as a hint value, and at MSB part
>>> +     * of HINT_VALUE_NSEC_BITS from nanoseconds.
>>> +     */
>>> +    int64_t secs = date->secs;
>>> +    int32_t nsec = date->nsec;
>>> +    uint64_t val = secs <= HINT_VALUE_SECS_MIN ? 0 :
>>> +            secs - HINT_VALUE_SECS_MIN;
>>> +    if (val >= HINT_VALUE_SECS_MAX)
>>> +        val = HINT_VALUE_SECS_MAX;
>>> +    val <<= HINT_VALUE_NSEC_BITS;
>>> +    val |= (nsec >> HINT_VALUE_NSEC_SHIFT) & HINT_VALUE_NSEC_MAX;
>>> +    return hint_create(MP_CLASS_DATETIME, val);
>>> +}
>>> +
>>
>> <stripped>
>>
>
> Patch increment here small (so far)
> ------------------------------------
> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
> index 110017853..2478498ba 100644
> --- a/src/box/tuple_compare.cc
> +++ b/src/box/tuple_compare.cc
> @@ -1550,8 +1550,8 @@ func_index_compare_with_key(struct tuple *tuple, 
> hint_t tuple_hint,
>   */
>  #define HINT_VALUE_SECS_BITS    36
>  #define HINT_VALUE_NSEC_BITS    (HINT_VALUE_BITS - HINT_VALUE_SECS_BITS)
> -#define HINT_VALUE_SECS_MAX    ((1LL << HINT_VALUE_SECS_BITS) - 1)
> -#define HINT_VALUE_SECS_MIN    (-(1LL << HINT_VALUE_SECS_BITS))
> +#define HINT_VALUE_SECS_MAX    ((1LL << (HINT_VALUE_SECS_BITS - 1)) - 1)
> +#define HINT_VALUE_SECS_MIN    (-(1LL << (HINT_VALUE_SECS_BITS - 1)))
>  #define HINT_VALUE_NSEC_SHIFT    (sizeof(int) * CHAR_BIT - 
> HINT_VALUE_NSEC_BITS)
>  #define HINT_VALUE_NSEC_MAX    ((1ULL << HINT_VALUE_NSEC_BITS) - 1)
>
> ------------------------------------
>
> But please see code moves which will be done in the next version of a 
> patchset, so all field and indices changes will become part of a 
> single patch.

Sure, I'll check out the new version once it's pushed.

>
> Regards,
> Timur

-- 
Serge Petrenko