[Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Nikita Pettik]

On 10 Feb 00:29, Alexander Turenko wrote:
> On Mon, Feb 03, 2020 at 12:52:10PM +0300, Mergen Imeev wrote:
> > On Tue, Jan 28, 2020 at 07:23:29PM +0300, Nikita Pettik wrote:
> > > On 23 Jan 16:40, Mergen Imeev wrote:
> > > > Hi! Thank you for review. My answers and new commit-message
> > > > below.
> > > 
> > > Hi,
> > > 
> > > I've a bit reworked your patch: splitted it into consistent sequence of
> > > independent patches and provided refactoring for sqlVdbeMemNumerify().
> > > Also I found that CAST boolean AS NUMBER didn't work properly: cast resulted
> > > in error meanshile cast boolean as integer is legal, so that as number should
> > > be legal as well. There's no more functional changes except for mentioned one.
> > > Please look at branch: https://github.com/tarantool/tarantool/commits/np/gh-4233-fix-number-field-type-in-sql
> > > 
> > > If it is OK to you please reply and I will push patches to master bracnch.
> > > Thanks. 
> > >  
> > Hi,
> > 
> > I'm sorry, but I do not like it. Since now it is yours
> > commits, I'll ask Sasha to move issues #4233 and #4463
> > to you. If you want a review, please ask Vlad.
> 
> I guess Nikita expects that you'll agree with splitting of the commits
> and show you the code to save a time on a discussion. Since you
> disagreed (this fast path fails), I think Nikita should provide

I've provided arguments in my review (as I always do).

> objections against your way explicitly. (However 'I do not like' is very
> weak argument, it is always better to provide some reasons explicitly.)
> 
> 'CAST boolean AS NUMBER' does not look as part of tasks Mergen solving
> here (#4233 and #4463).

Changes provided by Mergen look significant enough to consider them as
reworking of number type. So as long as cast blob to number is changed in
scope of patch, it seems to be ok to fix cast boolean to number as well.

>Aside of this, the commit that fixes it is named
> as 'sql: refactor sqlVdbeMemNumerify()', but it changes a user visible
> behaviour and so it is not pure refactoring.

Could you please explain why it "changes a user visible behaviour"? :)
What is more, commit message explicitly says:

"...allow conversion from boolean to number
    (since it is legal to convert boolean to integer, and in turn number
    type completely includes integer type)."

It is related only to this function, which in turn is unreachable.
If it is not clear enough, I'll fix a bit commit message.
Finally, if you wish to stress the word 'refactoring' implying its
'non-functional' origins..Well, as I already said, patch does not
introduce any functional changes (i.e. changes that can be tested), but in
current context 'refactoring' may seem a bit dubious. I guess replacing
it with 'rework' is enough to remove any doubts.

>It is also strange that it
> has no tests that will show how the behaviour is changed.

It's a consequence from your previous *wrong* observation: patch has no tests
since it doesn't change anything that can be tested. Tests which verify
boolean to number cast operation come with patch that introduces this
change (not the one we are speaking about).
 
> Changes related to Tarantool/SQL type system are very opaque now in the
> sense that we have no formal type system description we want to
> implement as result. Just 'fix that' and 'change this'. I think we
> should design the type system first (we should did it a year ago) and
> only then make some changes.

Well we (me, Kostja and Peter) had a long conversation last summer concerning
types in SQL (it is still available in mailing list). It is resulted in
following issues:
https://github.com/tarantool/tarantool/issues/3809
https://github.com/tarantool/tarantool/issues/4230

Alternatively, implicit casts can be completely removed (which matches
with ANSI on the one hand, but contradicts to user's exprience on
the other hand). Both options look acceptable to me.

Speaking of "type system", in fact there's well stablished plan. What we
need to do to finish it is (in terms of major issues):

 - remove implicit casts (or reconsider them; now they are a bit broken -
   see issues #3809 and #4230);
 - decide how to threat SCALAR type in SQL (on the discussion,
   see Re: [Tarantool-discussions] Implicit cast for COMPARISON);
 - implement MAP and ARRAY types (#4762 and #4763);
 - implement DECIMAL type (#4415).

Then we will be able to move to some more advanced features like varchars,
datetime, user-defined-cast operations and so forth (depending on customer's
requests).

Mergen's patch in particular resolves the legacy issues appeared due
to the absence of DOUBLE type in Tarantool. Owing to that "number"
type in SQL used to emulate DOUBLE type; since now double is on board,
we can revert that behaviour.

> CCed Kirill and Sergos to plan this
> activity (or decide that the current way to design it is okay).
> 
> WBR, Alexander Turenko.