Re: [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thank you for the patch. See 10 comments below.

1. I believe we do not have field type ANY in SQL. Also, I believe that 
to add
it to explicit cast we have to add it to SQL.

2. What this patch actually does? I mean, after this patch I still not able
to cast values to ANY:

tarantool> box.execute('SELECT CAST(1 AS ANY);')
---
- null
- 'At line 1 at or near position 18: keyword ''ANY'' is reserved. Please 
use double
   quotes if ''ANY'' is an identifier.'
...

On Tue, May 25, 2021 at 12:01:01PM +0300, Timur Safin wrote:
> For consistency sake it was decided to provide
> `CAST(anything TO ANY)` as allowed, but still noop.
> 
> Fuller explicit conversions table is described in the
> /doc/rfc/5910-consistent-sql-lua-types.md and here we
> represent only relevant to ANY.
> 
3. I still think that it is better to put RFC name instead of  this relative
link.

> Furthermore, there is no direct way in SQL to create literal and
> expression of ANY type, thus there is no defined CAST from ANY
> to anything else.
> 
4. There is no way to create literal, however you can use space with 
field type
ANY, right?

>               | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
>               +---+---+---+---+---+---+---+---+---+---+---+---+---+
>  0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
>  1.  unsigned |   | . | . | . | . |   |   | . |   |   |   |   | Y |
>  2.    string |   | . | . | . | . | . | . | . |   |   |   |   | Y |
>  4.    double |   | . | . | . | . |   |   | . |   |   |   |   | Y |
>  5.   integer |   | . | . | . | . |   |   | . |   |   |   |   | Y |
>  6.   boolean |   |   | . |   |   | . |   |   |   |   |   |   | Y |
>  7. varbinary |   |   | . |   |   |   | . |   |   |   |   |   | Y |
>  3.    number |   | . | . | . | . |   |   | . |   |   |   |   | Y |
>  9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
>  8.    scalar |   | . | . | . | . | . | . | . |   |   |   |   | Y |
>               +---+---+---+---+---+---+---+---+---+---+---+---+---+
> 
5. This strange order again.

6. Again, do you really need this table in commit-message? This time it 
is also
a lot simpler to just describe this explicit conversion rules using words.

> * We introduced new token ANY as known lexem to the Lemon parser.
>   While renamed prior %wildcard to ANYTHING;
> * Modified explicit conversion rules to allow ANY as _target_ in
>   conversion pair.
> 
> Relates to #5910, #6009
> Part of #4407
7. Wrong issue number. Also, I am again not sure that you should include
"Relates to" here.

> ---
>  extra/mkkeywordhash.c          | 3 ++-
>  src/box/sql/mem.c              | 2 ++
>  src/box/sql/parse.y            | 3 ++-
>  test/sql-tap/keyword1.test.lua | 2 +-
>  4 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> index 7480c0211..d343cf706 100644
> --- a/extra/mkkeywordhash.c
> +++ b/extra/mkkeywordhash.c
> @@ -60,6 +60,7 @@ static Keyword aKeywordTable[] = {
>    { "ALL",                    "TK_ALL",         true  },
>    { "ALTER",                  "TK_ALTER",       true  },
>    { "ANALYZE",                "TK_STANDARD",    true  },
> +  { "ANY",                    "TK_ID",          true  },
8. Why some types describes as TK_ID, and some as TK_<type name>?

>    { "AND",                    "TK_AND",         true  },
>    { "AS",                     "TK_AS",          true  },
>    { "ASC",                    "TK_ASC",         true  },
> @@ -178,7 +179,7 @@ static Keyword aKeywordTable[] = {
>    { "WITH",                   "TK_WITH",        true  },
>    { "WHEN",                   "TK_WHEN",        true  },
>    { "WHERE",                  "TK_WHERE",       true  },
> -  { "ANY",                    "TK_STANDARD",    true  },
> +  { "ANYTHING",               "TK_STANDARD",    true  },
9. Why not name it as TK_WILDCARD to avoid further misunderstanding?

>    { "ASENSITIVE",             "TK_STANDARD",    true  },
>    { "BLOB",                   "TK_STANDARD",    true  },
>    { "BINARY",                 "TK_ID",          true  },
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index d40366754..b4f1078ae 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -874,6 +874,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
>  		    (mem->flags & MEM_Subtype) != 0)
>  			return -1;
>  		return 0;
> +	case FIELD_TYPE_ANY:
> +		return 0;
>  	default:
>  		break;
>  	}
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index abc363951..e586d0181 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -270,7 +270,7 @@ columnlist ::= tcons.
>    QUERY KEY OFFSET RAISE RELEASE REPLACE RESTRICT
>    RENAME CTIME_KW IF ENABLE DISABLE
>    .
> -%wildcard ANY.
> +%wildcard ANYTHING.
>  
>  
>  // And "ids" is an identifer-or-string.
> @@ -1834,6 +1834,7 @@ typedef(A) ::= SCALAR . { A.type = FIELD_TYPE_SCALAR; }
>  typedef(A) ::= BOOL . { A.type = FIELD_TYPE_BOOLEAN; }
>  typedef(A) ::= BOOLEAN . { A.type = FIELD_TYPE_BOOLEAN; }
>  typedef(A) ::= VARBINARY . { A.type = FIELD_TYPE_VARBINARY; }
> +typedef(A) ::= ANY . { A.type = FIELD_TYPE_ANY; }
>  
>  /**
>   * Time-like types are temporary disabled, until they are
> diff --git a/test/sql-tap/keyword1.test.lua b/test/sql-tap/keyword1.test.lua
> index f9a9c6865..4855f3ebc 100755
> --- a/test/sql-tap/keyword1.test.lua
> +++ b/test/sql-tap/keyword1.test.lua
> @@ -242,7 +242,7 @@ for _, kw in ipairs(bannedkws) do
>      local query = 'CREATE TABLE '..kw..'(a INT PRIMARY KEY);'
>      if kw == 'end' or kw == 'match' or kw == 'release' or kw == 'rename' or
>         kw == 'replace' or kw == 'binary' or kw == 'character' or
> -       kw == 'smallint' then
> +       kw == 'smallint' or kw == 'any' then
>          test:do_catchsql_test(
>          "bannedkw1-"..kw..".1",
>          query, {
> -- 
> 2.29.2
> 
10. Why there is no tests?