Re: [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables

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

: From: Mergen Imeev <imeevma@tarantool.org>
: Subject: Re: [PATCH v2 0/3] sql: modify explicit and implicit conversion
: tables
: 
: Hi! Thank you for the patch-set. See 7 comments below.
: 
: 0. Please include you answers to my previous comments right here. Same
: about all other letters.

???

: 
: On Fri, Jun 11, 2021 at 10:48:10AM +0300, Timur Safin wrote:
: > Issues:
: > 	https://github.com/tarantool/tarantool/issues/4470
: > 	https://github.com/tarantool/tarantool/issues/6010
: > Branch: https://github.com/tarantool/tarantool/tree/tsafin/gh-4470-
: explicit-implicit-conversions-V2
: >
: > This patchset containes of 2 major part and 1 bonus. Major parts
: > are changes in explicit and implicit conversion tables, with all
: > accompanying tests we have so far (and 1 extensive test introduced).
: > The bonus part is for improvements in the testing system tap.lua
: > which became apparent during debugging of this patches.

: 1. Please divide this patch-set in two or three parts. I think the best
: way will
: be to divide it to three parts:
: i. Patch about tap.lua.
: ii. Patch-set about explicit casts.
: iii. Patch-set about implicit casts.

Ok, I'll exclude patch with trivial tap fix (which has helped me 
debug conversion tables test, but never mind).

I'll think about a reasonable way to divide patches for implicit
casts. Looks like there is easy way to extract part connected to
separate ticket. 

: 
: Combining ii and iii is also acceptable, but I think the approaches to
: ii and
: iii should be completely different. I don't see any connection between the
: tap.lua patch and all the other patches in this patchset.
: 
: I think it is strange, that only part of the patch-set is reviewed by
: me. Is it even possible for me to give LGTM to such patch-set?

Yes, this is normal - you react to the patch which you know,
and relay to others on other parts.
 
Let me explain it on some hypothetical example, not the current case.
Imagine that there is big refactoring of some subsystem (say 
introduction of cmake to luajit, with thousands of accompanying
tests added), and say you are aware of cmake internals, and review
it and send LGTM to only patches changing it, but are unaware of changes 
in luajit testing, then you just not ack this part, and refer to
others do their part.

Everything is possible, and circumstances may require attention of
various people, it's not problem if many people involved. 
At the end of a day this calls "team work". :)

: 
: >
: > Relates to #5910, #6009
: > Part of #4470
: > Fixes #6010
: >
: > Explicit table
: > ==============
: >
: > As we discovered harder way, there is no need to introduce so much changes
: > to the current explicit conversion table, because it's mostly compliant
: > already:
: > - most of changes we had to do was about `BOOLEAN` conversions, which are
: >   stricter now than before;
: > - there are addition of to `ANY` conversion, which we have decided to make
: >   behaving similar to `SCALAR` conversions;


: 2. As I previously said, I see no reason to include cast to field type
: ANY in this patch-set. Also, if we follow your logic, why you didn't
: added casts to array and map?

Explicit casts to ANY allowed as part of RFC ratified last quarter, and you
have been part of team developed that decision. It's ok if we implement something
you disagree with, but respect the team decision.

Arrays and maps not covered because they will be addressed later.

: 
: >
: > Those changes could be visualized via this table:
: >
: >
: >                   | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
: >                   +---+---+---+---+---+---+---+---+---+---+---+---+---+
: >      0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >      1.  unsigned |   | . | . | . | . | - |   | . |   |   |   |   | Y |
: >      2.    string |   | . | . | . | . | S | . | . |   |   |   |   | Y |
: >      4.    double |   | . | . | . | . | - |   | . |   |   |   |   | Y |
: >      5.   integer |   | . | . | . | . | - |   | . |   |   |   |   | Y |
: >      6.   boolean |   | - | Y | - | - | Y |   |   |   |   |   |   | Y |
: >      7. varbinary |   |   | . |   |   | - | . |   |   |   |   |   | Y |
: >      3.    number |   | . | . | . | . | - |   | . |   |   |   |   | Y |
: >      9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >     10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >     11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >     12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >      8.    scalar |   | . | . | . | . | S | . | . |   |   |   |   | Y |
: >                   +---+---+---+---+---+---+---+---+---+---+---+---+---+

: 3. Since you do not want to change numbers here, I suggest to arrange field
: types according to their number. 

I did not quite understand your sentence about numbers, could you please 
clarify? 

Also, I've already explained this strange order - this is exactly as table
described in RFC. I could reorder, but it would be harder to check against
original spec. 

: Also, why you described BOOLEAN, but didn't
: describe ANY and UUID?

Good point - I will accumulate and highlight everything touched by the patch,
table was copied from original `boolean` specific commit.

: 
: >
: > This table above represent all possible combinations of explicit cast
: > from type (row) to another type (column). Numbering of types might look
: > confusing (it's not sequential) but the order is what we see in the
: > consistency specification RFC, and we should live with that. Values are
: > actual enum values from the code. Unfortunately, changing of order is
: > almost impossible because of massive rework for the spec it would require.
: >
: > How to interpret this table, e.g.:
: > - for the explicit cast from `BOOLEAN` (6) to `BOOLEAN` (6) we should
: >   always allow cast (and make it noop), thus intersection is `Y` (yes);
: >
: > - `STRING` (2) to `BOOLEAN` (6) may work sometimes (if string represents
: >   TRUE or FALSE literals), but may fail otherwise, thus there is `S`
: (sometimes);
: >

: 4. "Sometimes" still sounds quite not right here.

Still not understand your complain here. For me it's quite ok.

: 
: > - We have prohibited majority of ther directions for `BOOLEAN` thus there
: are
: >   `-` (minus) in such cells, i.e. `BOOLEAN` (6) to `INTEGER` (5);
: >
: > - Untouched entries in the table marked with '.'. Assumption is -
: >   we already have correct conversion rule here activated well before;
: >
: > - Worth to mention that empty cell means that this conversion
: >   direction is prohibited.
: >

: 5. Does it mean that cast from UUID to UUID is prohibited?

Certainly it's not. Picture is incomplete, sorry for the confusion created.
I'll complete `boolean` only patch with all recent modifications. 
[Now you probably see how visualization helps to narrow down problems?]

: Also, I see no need
: of '.' since you sometimes use 'Y' or '-' or ' ' instead of it. I
: believe you
: should describe table for all explicit cast conversions here and not
: only for
: BOOLEAN.

Yes, will accumulate everything changed. And '.' means unmodified 'Y'/'S'
since prior state (pre-RFC implementation).

: 
: 
: > For obvious reasons those chnaged conversion rules made us modify
: > expected results for many tests - they are fixed.
: >
: > But to cover all possible conversion combinations (minus those, not yet
: > implemented in SQL types like DECIMAL, ARRAY and MAP) we have created
: > special test which check _all combinations of CASTs()_ and verifies
: > results against table rules defined in the RFC.
: >
: > There is e_casts.test.lua created for that purpose.


: 6. I still does not understand where this name came from. I believe you said
: something about renaming it.

Yep, will do.

: 
: >
: > Implicit table
: > ==============
: >
: > Similarly to explicit conversion rules we have update implicit conversion
: > rules, and this is 2nd part of patchset.
: >
: > We will not draw changed values of implicit conversion table (please see
: > it's final state in the consistemcy types RFC), but verbally we have
: > modified following directions of conversions:
: >   - string to/from double
: >   - double to/from integer
: >   - varbinary to/from string
: >
: > In addition to modifciation of all relevant test results, we have also
: > extended e_casts.test.lua introduced above for checking of all possible
: > directions of implicit conversions.
: >
: > Approach to check it though is different than for explicit cast, we do
: > not use simple expression with implicit cast activated (like we might be
: > using `CAST()` for explicit casts), but rather we insert value of original
: > type into columns of expected target type.

: 7. Implicit casts includes a lot more than just an insert, I believe.

Yes, according to SQL-2003 standard "Implicit type conversion can occur in expressions, fetch operations, single row select operations, inserts, deletes, and updates."

Insert looks the simplest way possible in the generated code to check cast 
against the expected result. I plan to extend test later, while adding more
casts for future types.

[At the moment there are no concise and clear type promotion rules when implicit
casts done in expressions. There is some hope that currently ongoing discussion
might provide such promotion rules as discussion side-effect, then we could 
add those rules to the test. Not ready for such today. Yet]

Regards,
Timur