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

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

: From: Mergen Imeev <imeevma@tarantool.org>
: Sent: Tuesday, June 1, 2021 5:03 PM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: tarantool-patches@dev.tarantool.org
: Subject: Re: [PATCH 0/3] sql: modify explicit conversion tables
: 
: 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.

¯\_(ツ)_/¯

Good you realize the possibility of various styles of commit messages
from different people. And that some people do have several decades prior
experience in American companies with some established English writing skills. 

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

There will be another table - implicit casts.

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

They are mostly equivalent, yes. 

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

Will include implicit soon.

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

This order is from illustrations of the spec. It's too late to redraw
all pictures there, but reordering here is easy. So we need to live 
with this order forever :(

: Also, I believe it makes
: sense to
: describe what "S", "Y" means and why some cells are empty.

Yes, good point. 

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

Because they already visualized in the RFC, and table will significantly
differ to version from spec, if we will remove all not yet implemented
columns and rows (and if we would reorder them in correct 
`enum field_type` order)

If we would keep this table more or less in the same form for all of
future updates in future commits for newer types, then this might
produce consistent picture along the way.

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

Yup

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