From: Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: "'Mergen Imeev'" <imeevma@tarantool.org> Cc: <tarantool-patches@dev.tarantool.org>, <alexander.turenko@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Date: Mon, 28 Jun 2021 02:29:20 +0300 [thread overview] Message-ID: <151a01d76bac$41c0f2b0$c542d810$@tarantool.org> (raw) In-Reply-To: <3d3690b6-8f40-ffd4-568b-c0acbc2d5f92@tarantool.org> : From: Mergen Imeev <imeevma@tarantool.org> : Subject: Re: [PATCH v2 0/3] sql: modify explicit and implicit conversion : tables : : 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. Ok, I'll exclude patch with trivial tap fix (which has helped me debug conversion tables test, but never mind). I'll think about a reasonable way to divide patches for implicit casts. Looks like there is easy way to extract part connected to separate ticket. : : 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? Yes, this is normal - you react to the patch which you know, and relay to others on other parts. Let me explain it on some hypothetical example, not the current case. Imagine that there is big refactoring of some subsystem (say introduction of cmake to luajit, with thousands of accompanying tests added), and say you are aware of cmake internals, and review it and send LGTM to only patches changing it, but are unaware of changes in luajit testing, then you just not ack this part, and refer to others do their part. Everything is possible, and circumstances may require attention of various people, it's not problem if many people involved. At the end of a day this calls "team work". :) : : > : > 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? Explicit casts to ANY allowed as part of RFC ratified last quarter, and you have been part of team developed that decision. It's ok if we implement something you disagree with, but respect the team decision. Arrays and maps not covered because they will be addressed later. : : > : > 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. I did not quite understand your sentence about numbers, could you please clarify? Also, I've already explained this strange order - this is exactly as table described in RFC. I could reorder, but it would be harder to check against original spec. : Also, why you described BOOLEAN, but didn't : describe ANY and UUID? Good point - I will accumulate and highlight everything touched by the patch, table was copied from original `boolean` specific commit. : : > : > 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. Still not understand your complain here. For me it's quite ok. : : > - 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? Certainly it's not. Picture is incomplete, sorry for the confusion created. I'll complete `boolean` only patch with all recent modifications. [Now you probably see how visualization helps to narrow down problems?] : 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. Yes, will accumulate everything changed. And '.' means unmodified 'Y'/'S' since prior state (pre-RFC implementation). : : : > 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. Yep, will do. : : > : > 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. Yes, according to SQL-2003 standard "Implicit type conversion can occur in expressions, fetch operations, single row select operations, inserts, deletes, and updates." Insert looks the simplest way possible in the generated code to check cast against the expected result. I plan to extend test later, while adding more casts for future types. [At the moment there are no concise and clear type promotion rules when implicit casts done in expressions. There is some hope that currently ongoing discussion might provide such promotion rules as discussion side-effect, then we could add those rules to the test. Not ready for such today. Yet] Regards, Timur
prev parent reply other threads:[~2021-06-27 23:29 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-11 7:48 Timur Safin via Tarantool-patches 2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines Timur Safin via Tarantool-patches 2021-06-20 18:57 ` Igor Munkin via Tarantool-patches 2021-06-23 21:01 ` Alexander Turenko via Tarantool-patches 2021-06-27 23:16 ` Timur Safin via Tarantool-patches 2021-06-29 16:21 ` Igor Munkin via Tarantool-patches 2021-06-30 6:49 ` Timur Safin via Tarantool-patches 2021-07-21 7:24 ` Igor Munkin via Tarantool-patches 2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table Timur Safin via Tarantool-patches 2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches 2021-06-25 21:26 ` Timur Safin via Tarantool-patches 2021-06-25 21:26 ` [Tarantool-patches] Отзыв: " Timur Safin via Tarantool-patches 2021-06-27 23:46 ` [Tarantool-patches] " Timur Safin via Tarantool-patches 2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 3/3] sql: updated implicit " Timur Safin via Tarantool-patches 2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches 2021-06-28 0:06 ` Timur Safin via Tarantool-patches 2021-06-20 18:52 ` [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Mergen Imeev via Tarantool-patches 2021-06-27 23:29 ` 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='151a01d76bac$41c0f2b0$c542d810$@tarantool.org' \ --to=tarantool-patches@dev.tarantool.org \ --cc=alexander.turenko@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tsafin@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit 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