[Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables
Mergen Imeev
imeevma at tarantool.org
Sun Jun 20 21:52:49 MSK 2021
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
>
More information about the Tarantool-patches
mailing list