Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce DOUBLE type

[Nikita Pettik <korablev@tarantool.org>]

On 26 Dec 19:42, Mergen Imeev wrote:
> Thank you for review! My answers and new commit-message
> below.

LGTM
 
> On Wed, Dec 25, 2019 at 01:50:46AM +0300, Nikita Pettik wrote:
> > On 21 Dec 19:03, imeevma@tarantool.org wrote:
> > > This patch introduces type DOUBLE in SQL.
> > > 
> > > Closes #3812
> > > Needed for #4233
> > > 
> > > @TarantoolBot document
> > > Title: Tarantool DOUBLE field type and DOUBLE type in SQL
> > > The DOUBLE field type was added to Tarantool mainly for adding the
> > > DOUBLE type to SQL.
> > > 
> > > In Lua, only non-integer numbers and CDATA of type DOUBLE can be
> > > inserted in this field. You cannot insert integers of type Lua
> > > NUMBER or CDATA of type int64 or uint64 in this field.
> > 
> > It would be nice to see justification for this ban.
> > 
> Added: "It was done this way to avoid unwanted implicit
> casts that could affect performance."
> 
> > > The same
> > > rules apply to key in get(), select(), update() and upsert()
> > > methods.
> > > 
> > > It is important to note that you can use the ffi.cast() function
> > > to cast numbers to CDATA of type DOUBLE. An example of this can be
> > > seen below.
> > > 
> > > Another very important point is that CDATA of type DOUBLE in lua
> > > can be used in arithmetic, but arithmetic for them does not work
> > > correctly. This comes from LuaJIT and most likely will not be
> > > fixed.
> > > 
> > > Example of usage in Lua:
> > > s = box.schema.space.create('s', {format = {{'d', 'double'}}})
> > > _ = s:create_index('ii')
> > > s:insert({1.1})
> > > ffi = require('ffi')
> > > s:insert({ffi.cast('double', 1)})
> > > s:insert({ffi.cast('double', tonumber('123'))})
> > > s:select(1.1)
> > > s:select({ffi.cast('double', 1)})
> > 
> > I'd also mention the way how double values are stored (their format:
> > mp_float or mp_double). It would allow to provide correct storage size
> > calculations.
> > 
> Added: "Values of this type are stored as MP_DOUBLE in
> msgpack. The size of the encoded value is always 9 bytes."
> 
> > > In SQL, DOUBLE type behavior is different due to implicit casting.
> > > In a column of type DOUBLE, the number of any supported type can
> > > be inserted. However, it is possible that the number that will be
> > > inserted will be different from that which is inserted due to the
> > > rules for casting to DOUBLE.
> > 
> > In addition, this patch makes type of floating point literals
> > be double (not number).
> > 
> Added: "In addition, after this patch, all floating point
> literals will be recognized as DOUBLE. Prior to that, they
> were considered as NUMBER."
> 
> > > Example of usage in SQL:
> > > box.execute('CREATE TABLE t (d DOUBLE PRIMARY KEY);')
> > > box.execute('INSERT INTO t VALUES (10), (-2.0), (3.3);')
> > > box.execute('SELECT * FROM t;')
> > > box.execute('SELECT d / 100 FROM t;')
> > > box.execute('SELECT * from t WHERE d < 15;')
> > > box.execute('SELECT * from t WHERE d = 3.3;')
> > > ---
> > > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > > index 407b42e..df3f0d8 100644
> > > --- a/src/box/sql/vdbemem.c
> > > +++ b/src/box/sql/vdbemem.c
> > > @@ -741,6 +741,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
> > >  		    (pMem->flags & MEM_UInt) == 0)
> > >  			return -1;
> > >  		return 0;
> > > +	case FIELD_TYPE_DOUBLE:
> > >  	case FIELD_TYPE_NUMBER:
> > >  		return sqlVdbeMemRealify(pMem);
> > >  	case FIELD_TYPE_VARBINARY:
> > 
> > Are you going to fix CAST TO NUMBER in a separate follow-up?
> > 
> Yes, I will fix this as part of #4233 issue.
> 
> > PS quite brief and meanwhile nice patch. Thanks.
> > 
> Thank you :)
> 
> New commit-message:
> 
> sql: introduce DOUBLE type
> 
> This patch introduces type DOUBLE in SQL.
> 
> Closes #3812
> Needed for #4233
> 
> @TarantoolBot document
> Title: Tarantool DOUBLE field type and DOUBLE type in SQL
> The DOUBLE field type was added to Tarantool mainly for adding the
> DOUBLE type to SQL. Values of this type are stored as MP_DOUBLE in
> msgpack. The size of the encoded value is always 9 bytes.
> 
> In Lua, only non-integer numbers and CDATA of type DOUBLE can be
> inserted in this field. You cannot insert integers of type Lua
> NUMBER or CDATA of type int64 or uint64 in this field. The same
> rules apply to key in get(), select(), update() and upsert()
> methods. It was done this way to avoid unwanted implicit casts
> that could affect performance.
> 
> It is important to note that you can use the ffi.cast() function
> to cast numbers to CDATA of type DOUBLE. An example of this can be
> seen below.
> 
> Another very important point is that CDATA of type DOUBLE in lua
> can be used in arithmetic, but arithmetic for them does not work
> correctly. This comes from LuaJIT and most likely will not be
> fixed.
> 
> Example of usage in Lua:
> s = box.schema.space.create('s', {format = {{'d', 'double'}}})
> _ = s:create_index('ii')
> s:insert({1.1})
> ffi = require('ffi')
> s:insert({ffi.cast('double', 1)})
> s:insert({ffi.cast('double', tonumber('123'))})
> s:select(1.1)
> s:select({ffi.cast('double', 1)})
> 
> In SQL, DOUBLE type behavior is different due to implicit casting.
> In a column of type DOUBLE, the number of any supported type can
> be inserted. However, it is possible that the number that will be
> inserted will be different from that which is inserted due to the
> rules for casting to DOUBLE. In addition, after this patch, all
> floating point literals will be recognized as DOUBLE. Prior to
> that, they were considered as NUMBER.
> 
> Example of usage in SQL:
> box.execute('CREATE TABLE t (d DOUBLE PRIMARY KEY);')
> box.execute('INSERT INTO t VALUES (10), (-2.0), (3.3);')
> box.execute('SELECT * FROM t;')
> box.execute('SELECT d / 100 FROM t;')
> box.execute('SELECT * from t WHERE d < 15;')
> box.execute('SELECT * from t WHERE d = 3.3;')
>