Re: [Tarantool-patches] [PATCH v3 6/9] box, datetime: datetime comparison for indices

[Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

> From: Serge Petrenko <sergepetrenko@tarantool.org>
> Subject: Re: [Tarantool-patches] [PATCH v3 6/9] box, datetime: datetime
> comparison for indices
> 
> 
> 
> 02.08.2021 03:41, 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
> 
> Hi! Thanks for the patch!
> 
> Please find 2 comments below.
> 
...
> > @@ -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)
> > +#define HINT_VALUE_SECS_MIN	(-(1LL << HINT_VALUE_SECS_BITS))
> > +#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,24 @@ hint_uuid_raw(const char *data)
> >   	return hint_create(MP_CLASS_UUID, val);
> >   }
> >
> > +static inline hint_t
> > +hint_datetime(struct datetime_t *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;
> > +	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);
> > +}
> > +
> I like the idea with having hints for some "near" dates.
> 
> You just need to assign the same HINT_VALUE_MAX hint to every datetime value
> with date->secs >= HINT_VALUE_SECS_MAX. Otherwise the comparison would
> make mistakes (judging by hint values only) for such far away dates.

Yes, thanks for the note, will make sure that values which exceed selected
36-bit will stick to the maximum value.

> 
> >   static inline uint64_t
> >   hint_str_raw(const char *s, uint32_t len)
> >   {


> > @@ -1812,6 +1858,11 @@ field_hint_scalar(const char *field, struct coll
> *coll)
> >   		}
> >   		case MP_UUID:
> >   			return hint_uuid_raw(field);
> > +		case MP_DATETIME:
> > +		{
> > +			struct datetime_t date;
> > +			return hint_datetime(datetime_unpack(&field, len, &date));
> > +		}
> 
> But you don't allow datetime in SCALAR fields, as I see in the previous
> commit.
> So you don't need to account for datetime in scalar hints.

That's bug - every type is scalar type, unless it's collection (map or array)
so datetime should be considered as scalar. That was oversight if not. 
Thanks!

> 
> >   		default:
> >   			unreachable();
> >   		}
 
> --
> Serge Petrenko

Thanks,
Timur