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

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.

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?

> 
> 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?

> 
> 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. Also, why you described BOOLEAN, but didn't
describe ANY and UUID?

> 
> 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.

> - 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? 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.


> 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.

> 
> 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.

> 
> Relates to #5910, #6009
> Closes #4470
> 
> 
> Bonus - better error reporting in tap tests
> ===========================================
> 
> Also, during debugging of explicit/implicitit conversions test it was
> discovered the harder way, that TAP infrastructure does not report correctly
> origin source line of and error, but rather report :0 line as location.
> This has been fixed.
> 
> Closes #6134
> 
> 
> Timur Safin (3):
>   test: corrected reported error lines
>   sql: updated explicit conversion table
>   sql: updated implicit conversion table
> 
>  extra/mkkeywordhash.c                |   3 +-
>  src/box/sql/mem.c                    | 188 +++--------
>  src/box/sql/mem.h                    |   6 -
>  src/box/sql/parse.y                  |   9 +-
>  src/box/sql/util.c                   |   3 +-
>  src/box/sql/vdbe.c                   |   8 +-
>  src/box/sql/vdbeaux.c                |   2 +-
>  src/lua/tap.lua                      |   2 +-
>  test/sql-tap/cse.test.lua            |  12 +-
>  test/sql-tap/e_casts.test.lua        | 474 +++++++++++++++++++++++++++
>  test/sql-tap/e_select1.test.lua      |   2 +-
>  test/sql-tap/in1.test.lua            |   8 +-
>  test/sql-tap/in4.test.lua            |  19 +-
>  test/sql-tap/misc3.test.lua          |   2 +-
>  test/sql-tap/numcast.test.lua        |   2 +-
>  test/sql-tap/tkt-9a8b09f8e6.test.lua |  40 +--
>  test/sql/boolean.result              |  38 +--
>  test/sql/types.result                | 132 ++++----
>  18 files changed, 654 insertions(+), 296 deletions(-)
>  create mode 100755 test/sql-tap/e_casts.test.lua
> 
> -- 
> 2.29.2
>