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
Subject: Re: [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables
Date: Tue, 1 Jun 2021 17:02:34 +0300	[thread overview]
Message-ID: <4c9c6d3f-78e7-d19e-bb54-868d3b469d13@tarantool.org> (raw)
In-Reply-To: <cover.1621894609.git.tsafin@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
> 

  parent reply	other threads:[~2021-06-01 14:02 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-25  9:00 Timur Safin via Tarantool-patches
2021-05-25  9:01 ` [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables Timur Safin via Tarantool-patches
2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
2021-06-02 21:03     ` Timur Safin via Tarantool-patches
2021-06-02 21:10     ` Timur Safin via Tarantool-patches
2021-05-25  9:01 ` [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions Timur Safin via Tarantool-patches
2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
2021-06-02 21:04     ` Timur Safin via Tarantool-patches
2021-05-25  9:01 ` [Tarantool-patches] [PATCH 3/3] sql: introduced explicit casts test e_casts.test.lua Timur Safin via Tarantool-patches
2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
2021-06-02 21:04     ` Timur Safin via Tarantool-patches
2021-06-01 14:02 ` Mergen Imeev via Tarantool-patches [this message]
2021-06-02 21:04   ` [Tarantool-patches] [PATCH 0/3] sql: modify explicit 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=4c9c6d3f-78e7-d19e-bb54-868d3b469d13@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=tsafin@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 0/3] sql: modify explicit 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