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

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

Hi! Thank you for the patch-set. See 6 comments below.

0. In general, I want to say that I do not like your style of 
commit-messages.
I think there are too many unnecessary words and constructions in it. But
that's just my opinion. I have not seen any rules prohibiting this.

1. Why "tables" in name of the patch? I see only one table.

On Tue, May 25, 2021 at 12:00:59PM +0300, Timur Safin wrote:
> 
> Recent RFC "Consistent Lua/SQL types" (#6009) defined ideal explicit
> and implicit conversion tables we would like to have for all current 
> and future Taranool SQL types. 
> 
2. Why do you need number of pull-request here? I mean, you already have 
a name
of RFC.

> This patchset modifies explict conversion tables, and implicit 
3. Here should be "explicit" I believe.

> conversion table to come soon. The ideal picture would be as below:
> 

>               | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
>               +---+---+---+---+---+---+---+---+---+---+---+---+---+
>  0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
>  1.  unsigned |   | Y | Y | Y | Y |   |   | Y |   |   |   |   | Y |
>  2.    string |   | S | Y | S | S | S | Y | S |   |   |   |   | Y |
>  4.    double |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
>  5.   integer |   | S | Y | Y | Y |   |   | Y |   |   |   |   | Y |
>  6.   boolean |   |   | Y |   |   | Y |   |   |   |   |   |   | Y |
>  7. varbinary |   |   | Y |   |   |   | Y |   |   |   |   |   | Y |
>  3.    number |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
>  9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
>  8.    scalar |   | S | Y | S | S | S | S | S |   |   |   |   | Y |
>               +---+---+---+---+---+---+---+---+---+---+---+---+---+
> 
4. Why there is such strange order in table? Also, I believe it makes 
sense to
describe what "S", "Y" means and why some cells are empty.

> Please pay attention that we omit DECIMAL, UUID, SCALAR and MAP rows
> and columns, as they not yet fully supported by Tarantool SQL. Once 
> their support will be landed we will modify conversion table and 
> tests (which we also introducing with current patchset).
> 
5. Why not drop them if they are omitted? I mean, "modify" in name of 
the letter
means that you change existing rules.

6. Please include links to issue and branch in cover-letter.

> 
> Timur Safin (3):
>   sql: fixes for boolean expressions in explicit converstion tables
>   sql: enabled ANY as target for explicit conversions
>   sql: introduced explicit casts test e_casts.test.lua
> 
>  extra/mkkeywordhash.c           |   3 +-
>  src/box/sql/mem.c               |  39 +---
>  src/box/sql/parse.y             |   3 +-
>  test/sql-tap/cse.test.lua       |  12 +-
>  test/sql-tap/e_casts.test.lua   | 355 ++++++++++++++++++++++++++++++++
>  test/sql-tap/e_select1.test.lua |   2 +-
>  test/sql-tap/in1.test.lua       |  16 +-
>  test/sql-tap/keyword1.test.lua  |   2 +-
>  test/sql-tap/misc3.test.lua     |   2 +-
>  test/sql/boolean.result         |  38 +---
>  test/sql/types.result           |  14 +-
>  11 files changed, 390 insertions(+), 96 deletions(-)
>  create mode 100755 test/sql-tap/e_casts.test.lua
> 
> -- 
> 2.29.2
>