Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Timur Safin <tsafin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, alexander.turenko@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables
Date: Sun, 20 Jun 2021 21:52:49 +0300	[thread overview]
Message-ID: <3d3690b6-8f40-ffd4-568b-c0acbc2d5f92@tarantool.org> (raw)
In-Reply-To: <cover.1623396615.git.tsafin@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
> 

  parent reply	other threads:[~2021-06-20 18:52 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-11  7:48 Timur Safin via Tarantool-patches
2021-06-11  7:48 ` [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines Timur Safin via Tarantool-patches
2021-06-20 18:57   ` Igor Munkin via Tarantool-patches
2021-06-23 21:01     ` Alexander Turenko via Tarantool-patches
2021-06-27 23:16     ` Timur Safin via Tarantool-patches
2021-06-29 16:21       ` Igor Munkin via Tarantool-patches
2021-06-30  6:49         ` Timur Safin via Tarantool-patches
2021-07-21  7:24           ` Igor Munkin via Tarantool-patches
2021-06-11  7:48 ` [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table Timur Safin via Tarantool-patches
2021-06-20 18:52   ` Mergen Imeev via Tarantool-patches
2021-06-25 21:26     ` Timur Safin via Tarantool-patches
2021-06-25 21:26     ` [Tarantool-patches] Отзыв: " Timur Safin via Tarantool-patches
2021-06-27 23:46       ` [Tarantool-patches] " Timur Safin via Tarantool-patches
2021-06-11  7:48 ` [Tarantool-patches] [PATCH v2 3/3] sql: updated implicit " Timur Safin via Tarantool-patches
2021-06-20 18:52   ` Mergen Imeev via Tarantool-patches
2021-06-28  0:06     ` Timur Safin via Tarantool-patches
2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches [this message]
2021-06-27 23:29   ` [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Timur Safin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3d3690b6-8f40-ffd4-568b-c0acbc2d5f92@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=tsafin@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox