[tarantool-patches] Re: [PATCH 0/3 v2] sql: add test for indexed char in sub subquery

[n.pettik]

> On 5 Dec 2018, at 23:35, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> Vova, please, ignore. It was accidentally sent to you.
> 
> Nikita, please, review.

I didn’t receive mail since there were issues with our mailing list,
so I looked at patch-set on GitHub.

> 
> On 05/12/2018 02:47, Roman Khabibov wrote:
>> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3580-err-msg-pathjoin
>> Issue: https://github.com/tarantool/tarantool/issues/3580
>> Roman Khabibov (2):
>>   sql: add CHAR description without length


>     Fix an ability to describe CHAR without specifying the

Not ‘fix’ but rather ‘add’. Otherwise, it sounds wierd.

>     length as it is allowed by standard. It was
>     accidentally broken by this commit:
>     7752cdfd31f9956a4d6bb0f306c561a0ac73e7ab
>     
>     Needed for #3616

It is not needed for #3616 - this commit and issue are not connected.
Issue disappeared after static types were introduced. It is ok that
you added test case on this issue, but these two problems are not related.

Consider refactoring:

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index f5e86fba1..d56fce451 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1485,26 +1485,22 @@ typedef(A) ::= DATE . { A.type = AFFINITY_REAL; }
 typedef(A) ::= TIME . { A.type = AFFINITY_REAL; }
 typedef(A) ::= DATETIME . { A.type = AFFINITY_REAL; }
 
-%type char_len {int}
-typedef(A) ::= CHAR char_len(B) . {
+typedef(A) ::= CHAR . {
   A.type = AFFINITY_TEXT;
-  (void) B;
 }
 
-%type vchar_len {int}
-typedef(A) ::= VARCHAR vchar_len(B) . {
-  A.type = AFFINITY_TEXT;
+char_len(A) ::= LP INTEGER(B) RP . {
+  (void) A;
   (void) B;
 }
 
-char_len(A) ::=  . {
-  (void) A;
+typedef(A) ::= CHAR char_len(B) . {
+  A.type = AFFINITY_TEXT;
+  (void) B;
 }
 
-char_len(A) ::=  vchar_len(A) .
-
-vchar_len(A) ::= LP INTEGER(B) RP . {
-  (void) A;
+typedef(A) ::= VARCHAR char_len(B) . {
+  A.type = AFFINITY_TEXT;
   (void) B;
 }

In this case code looks a bit more straightforward IMHO.

>>   sql: add test for indexed char in sub subquery

You don’t need to drop and re-create table t2 -
it doesn’t change in half cases. You should drop it
only when you change format of table, but in other
cases it doesn’t affect anything.

>> Vladislav Shpilevoy (1):
>>   sql: store CHAR|VARCHAR len as integer, not type_def

This is OK.