From: Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: "'Mergen Imeev'" <imeevma@tarantool.org> Cc: <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Date: Thu, 3 Jun 2021 00:04:09 +0300 [thread overview] Message-ID: <052f01d757f2$d50cd890$7f2689b0$@tarantool.org> (raw) In-Reply-To: <4c9c6d3f-78e7-d19e-bb54-868d3b469d13@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 : >
prev parent reply other threads:[~2021-06-02 21:04 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 ` [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Mergen Imeev via Tarantool-patches 2021-06-02 21:04 ` Timur Safin via Tarantool-patches [this message]
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='052f01d757f2$d50cd890$7f2689b0$@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